Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dhdl plot for the TI #110

Merged
merged 26 commits into from
Oct 2, 2020
Merged

dhdl plot for the TI #110

merged 26 commits into from
Oct 2, 2020

Conversation

xiki-tempula
Copy link
Collaborator

@xiki-tempula xiki-tempula commented Oct 1, 2020

Partially addresses #73

I have ported the dhdl plot for TI from the alchemical analysis.

Based on user experience, I have changed a few things.

  • The user could now set custom label to the figure
  • The x axis is elevated a bit to avoid bumping to the dhdl
  • The user could now custom the color of the plot

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #110 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   97.30%   97.56%   +0.25%     
==========================================
  Files          14       15       +1     
  Lines         743      822      +79     
  Branches      150      167      +17     
==========================================
+ Hits          723      802      +79     
  Misses          5        5              
  Partials       15       15              
Impacted Files Coverage Δ
src/alchemlyb/estimators/mbar_.py 96.15% <ø> (ø)
src/alchemlyb/visualisation/__init__.py 100.00% <ø> (ø)
src/alchemlyb/estimators/ti_.py 100.00% <100.00%> (ø)
src/alchemlyb/visualisation/ti_dhdl.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3d240b...f7853fc. Read the comment docs.

@xiki-tempula
Copy link
Collaborator Author

@orbeckst I'm sorry that I felt that I cannot bump the test coverage to any higher.
I have ported the code from the alchemical analysis and modified it such that it works in py38.

However, there are two regions that I'm not quite sure of what it does.

    def getInd(r=ri, z=[0]):
        primo = r[0]
        min_dl = ndx * 0.02 * 2 ** (primo > 10)
        if dl_mat[primo].max() < min_dl:
            return z
        for i in r:
            for j in range(len(xs)):
                if dl_mat[i, j] > min_dl:
                    z.append(j)
                    return getInd(ri[j:], z)

This function exists to make sure that there won't be too many ticks in the x axis.
I have tested this function with

    ti_coul.dhdl = pd.DataFrame.from_dict(
        {'fep': range(100)},
        orient='index',
        columns=np.arange(100)/100).T
    assert isinstance(plot_ti_dhdl(ti_coul),
               matplotlib.axes.Axes)

but cannot get it fully covered.

The other part is

    if ndx > 1:
        lenticks = len(ax.get_ymajorticklabels()) - 1
        if min_y < 0: lenticks -= 1
        if lenticks < 5:
            from matplotlib.ticker import AutoMinorLocator as AML
            ax.yaxis.set_minor_locator(AML())

Which my understanding is to deal with the case when there are too tittle ticks in the x axis.
I have tested it with

    ti_coul.dhdl = pd.DataFrame.from_dict(
        {'fep': range(3)},
        orient='index',
        columns=np.arange(3)/3).T
    assert isinstance(plot_ti_dhdl(ti_coul),
               matplotlib.axes.Axes)

but cannot get it fully covered.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work and kudos for pushing up coverage as high as you did – it's hard with plots!

Given that you identified the code pieces that are hard to test, mark them explicitly as not included in coverage. Something like

# pragma: no cover

You might have to find out how exactly to do this (we do it in MDAnalysis) but with this you explicitly acknowledge that certain code is untested and then it's removed from the coverage calculation.

@xiki-tempula
Copy link
Collaborator Author

@orbeckst Thank you for the advice. I have commented out the relevant lines with # pragma: no cover.

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adjusting coverage. I just saw two things to change: raise exceptions instead of using assertions for input checking – this was probably already in alchemical-analysis but should certainly not be in alchemlyb.

EDIT: Please also add an entry to CHANGES.

Otherwise, this looks excellent.

src/alchemlyb/visualisation/ti_dhdl.py Outdated Show resolved Hide resolved
src/alchemlyb/visualisation/ti_dhdl.py Outdated Show resolved Hide resolved
@orbeckst orbeckst merged commit 79e2a91 into alchemistry:master Oct 2, 2020
@orbeckst
Copy link
Member

orbeckst commented Oct 2, 2020

Thank you @xiki-tempula !! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants