Allow dF-state to be plotted #112

xiki-tempula · 2020-10-04T18:22:26Z

I have ported the dF_state from the alchemical analysis.
I have modified the x axis a bit to make it looks more comfortable.
#73

codecov · 2020-10-04T18:28:30Z

Codecov Report

Merging #112 into master will decrease coverage by 0.22%.
The diff coverage is 95.68%.

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   97.56%   97.33%   -0.23%     
==========================================
  Files          15       16       +1     
  Lines         822      939     +117     
  Branches      167      205      +38     
==========================================
+ Hits          802      914     +112     
  Misses          5        5              
- Partials       15       20       +5

Impacted Files	Coverage Δ
src/alchemlyb/visualisation/__init__.py	`100.00% <ø> (ø)`
src/alchemlyb/visualisation/dF_state.py	`95.68% <95.68%> (ø)`

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 79e2a91...a3e3264. Read the comment docs.

xiki-tempula · 2020-10-04T20:07:32Z

@orbeckst Thank you for the review. I'm a bit confused as to the yellow lines in the code coverage (https://codecov.io/gh/alchemistry/alchemlyb/pull/112/diff?src=pr&el=tree#diff-c3JjL2FsY2hlbWx5Yi92aXN1YWxpc2F0aW9uL2RGX3N0YXRlLnB5) as in the debug mode I have checked to makes sure that these lines are indeed executed in the test.

orbeckst

I thought I had submitted this review earlier but GitHub told me that it was still pending so I am submitting it now. Sorry if this sounds confusing to you but it sounded to me that you already had seen a review from me.

Please see comments inline to make the code a bit clearer. Thanks!

orbeckst · 2020-10-04T19:06:40Z

docs/visualisation.rst

+--------------------------------------------
+Another way of assessing the quality of free energy estimate would be comparing
+the free energy difference between adjacent lambda states (dF) using different
+estimators. The function :func:`~alchemlyb.visualisation.plot_dF_state` can


reference Klimovich paper

Suggested change

estimators. The function :func:`~alchemlyb.visualisation.plot_dF_state` can

estimators [Klimovich2015]_. The function :func:`~alchemlyb.visualisation.plot_dF_state` can

docs/visualisation.rst

orbeckst · 2020-10-04T19:09:09Z

docs/visualisation.rst

+
+Will give a plot looks like this
+
+.. image:: images/dF_states.png


You can also use the .. figure:: directive, which also allows you put down a caption https://docutils.sourceforge.io/docs/ref/rst/directives.html#figure

orbeckst · 2020-10-04T19:10:02Z

docs/visualisation/alchemlyb.visualisation.plot_dF_state.rst

+=======================================
+
+The function :func:`~alchemlyb.visualisation.plot_dF_state` allows the user to
+plot and compare the dF from various kinds of :class:`~alchemlyb.estimators`.


Write out "dF" as free energy difference between states ("dF") to make the text self contained.

orbeckst · 2020-10-04T19:12:14Z

docs/visualisation.rst

+    >>> mbar_coul = MBAR().fit(u_nk_coul)
+    >>> mbar_vdw = MBAR().fit(u_nk_vdw)
+
+    >>> dhdl_data = [(ti_coul, ti_vdw),


It's not really dH/dl, is it? I find the variable name misleading. Maybe just call it estimators?

orbeckst · 2020-10-04T19:13:42Z

docs/visualisation/alchemlyb.visualisation.plot_dF_state.rst

+plot and compare the dF from various kinds of :class:`~alchemlyb.estimators`.
+
+To compare the dF states of a single alchemical transformation among various
+:class:`~alchemlyb.estimators`, the user can pass a list of `dhdl_data`. (e.g.


Instead of dhdl_data just call it estimators.

Also, in reST, normally code is marked up with double back-quotes. Single backquotes are the "default role".

@orbeckst Thank you for the review. I was hoping to show a list with reference to different estimators instead of a line of code.

estimators = [:class:`~alchemlyb.estimators.TI`, :class:`~alchemlyb.estimators.BAR`, :class:`~alchemlyb.estimators.MBAR`]

I would imagine that doing this would not allow the user to click TI to show the page of alchemlyb.estimators.TI

orbeckst · 2020-10-04T19:16:56Z

docs/visualisation/alchemlyb.visualisation.plot_dF_state.rst

+:class:`~alchemlyb.estimators.BAR`), (:class:`~alchemlyb.estimators.MBAR`,
+:class:`~alchemlyb.estimators.MBAR`)])
+
+The figure could be plotted in `portrait` or `landscape` mode by setting the


Markup: strings should just be simple text with quotation marks or emphasize. Code uses double back tics. We have in the past used the default role (single back tics) for kwarg names. So

*portrait* or *landscape* `orientation` `nb`

orbeckst · 2020-10-04T19:17:26Z

docs/visualisation/alchemlyb.visualisation.plot_dF_state.rst

+the estimators differently. The unit in the y axis could be labelled to other
+units by setting `units`, which by default is kcal/mol.
+
+Please check :ref:`How to plot dF states <plot_dF_states>` for usage.


Suggested change

Please check :ref:`How to plot dF states <plot_dF_states>` for usage.

Please check :ref:`How to plot dF states <plot_dF_states>` for a complete example.

orbeckst · 2020-10-05T06:43:06Z

src/alchemlyb/visualisation/dF_state.py

+        else:  # pragma: no cover
+            pass


Just delete

orbeckst · 2020-10-05T06:43:19Z

src/alchemlyb/visualisation/dF_state.py

+            else:  # pragma: no cover
+                pass


Just delete

orbeckst · 2020-10-05T06:43:34Z

src/alchemlyb/visualisation/dF_state.py

+    else:  # pragma: no cover
+        pass


just delete

orbeckst · 2020-10-05T06:45:30Z

I don't know why coverage thinks that the elif statements are not fully executed, given that the code blocks themselves are shown green. I wouldn't worry about it.

Btw, there are some else: pass blocks that can just be removed to save a few bytes.

xiki-tempula · 2020-10-05T09:04:24Z

@orbeckst Thank you for the insight. I initially thought that the reason that elif is yellow is because of the lack of the else statement and add the else: pass, which didn't seems to help.

caption took from Klimovich

orbeckst · 2020-10-05T16:42:55Z

FYI @dotsdl @davidlmobley – I changed the repo settings to allow administrators to override the required checks so that we can still merge even if the coverage patch diff is below threshold. In the case here, coverage indicates that 3 if or elif clauses are only partially tested even while all corresponding code blocks are being executed. I find such behavior puzzling and don't understand what else could be tested. Diff coverage is still > 95% so I will just merge it as it is.

orbeckst

@xiki-tempula I forgot: please add an entry to CHANGES (and mention issue #73 in the message; also add #73 after the last where you added plotting

Enhancements
  - Allow the overlap matrix of the MBAR estimator to be plotted. (issue #73, PR #107)
  - Allow the dhdl of the TI estimator to be plotted. (issue #73, PR #110)
  - Allow dF-state to be plotted (issue #73, PR #112)

xiki-tempula · 2020-10-05T16:54:34Z

@orbeckst I'm sorry for the omission, changelog updated.

orbeckst · 2020-10-05T19:18:05Z

Thank you @xiki-tempula !

xiki-tempula added 2 commits October 3, 2020 12:04

Create dF_state.py

c05576e

add df_state

28c5034

xiki-tempula added 4 commits October 4, 2020 19:36

bump test

69c6b06

bump test

ad49760

name error

bbc3b25

fix the reference

e152a5e

orbeckst requested changes Oct 5, 2020

View reviewed changes

orbeckst reviewed Oct 5, 2020

View reviewed changes

src/alchemlyb/visualisation/dF_state.py Outdated

Comment on lines 187 to 188

else: # pragma: no cover

pass

Copy link

Member

orbeckst Oct 5, 2020

Choose a reason for hiding this comment

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

Just delete

orbeckst reviewed Oct 5, 2020

View reviewed changes

src/alchemlyb/visualisation/dF_state.py Outdated

Comment on lines 160 to 161

else: # pragma: no cover

pass

Copy link

Member

orbeckst Oct 5, 2020

Choose a reason for hiding this comment

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

Just delete

orbeckst reviewed Oct 5, 2020

View reviewed changes

src/alchemlyb/visualisation/dF_state.py Outdated

Comment on lines 207 to 208

else: # pragma: no cover

pass

Copy link

Member

orbeckst Oct 5, 2020

Choose a reason for hiding this comment

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

just delete

xiki-tempula added 3 commits October 5, 2020 10:16

check coverage

0c4da27

Figure caption

cb922b7

caption

b3a7381

caption took from Klimovich

orbeckst approved these changes Oct 5, 2020

View reviewed changes

orbeckst self-assigned this Oct 5, 2020

orbeckst requested changes Oct 5, 2020

View reviewed changes

xiki-tempula added 2 commits October 5, 2020 17:51

Update CHANGES

86b22ed

Update CHANGES

a3e3264

orbeckst approved these changes Oct 5, 2020

View reviewed changes

orbeckst merged commit 0c6545f into alchemistry:master Oct 5, 2020

xiki-tempula deleted the dF_state branch October 5, 2020 19:24

xiki-tempula mentioned this pull request Feb 28, 2021

Implement the workflow pipeline #114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow dF-state to be plotted #112

Allow dF-state to be plotted #112

xiki-tempula commented Oct 4, 2020 •

edited

Loading

codecov bot commented Oct 4, 2020 •

edited

Loading

xiki-tempula commented Oct 4, 2020

orbeckst left a comment

orbeckst Oct 4, 2020

orbeckst Oct 4, 2020

orbeckst Oct 4, 2020

orbeckst Oct 4, 2020

orbeckst Oct 4, 2020

xiki-tempula Oct 5, 2020 •

edited

Loading

orbeckst Oct 4, 2020

orbeckst Oct 4, 2020

orbeckst Oct 5, 2020

orbeckst Oct 5, 2020

orbeckst Oct 5, 2020

orbeckst commented Oct 5, 2020

xiki-tempula commented Oct 5, 2020

orbeckst commented Oct 5, 2020

orbeckst left a comment

xiki-tempula commented Oct 5, 2020

orbeckst commented Oct 5, 2020

	estimators. The function :func:`~alchemlyb.visualisation.plot_dF_state` can
	estimators [Klimovich2015]_. The function :func:`~alchemlyb.visualisation.plot_dF_state` can


		Will give a plot looks like this

		.. image:: images/dF_states.png

	Please check :ref:`How to plot dF states <plot_dF_states>` for usage.
	Please check :ref:`How to plot dF states <plot_dF_states>` for a complete example.

Allow dF-state to be plotted #112

Allow dF-state to be plotted #112

Conversation

xiki-tempula commented Oct 4, 2020 • edited Loading

codecov bot commented Oct 4, 2020 • edited Loading

Codecov Report

xiki-tempula commented Oct 4, 2020

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiki-tempula Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Oct 5, 2020

xiki-tempula commented Oct 5, 2020

orbeckst commented Oct 5, 2020

orbeckst left a comment

Choose a reason for hiding this comment

xiki-tempula commented Oct 5, 2020

orbeckst commented Oct 5, 2020

xiki-tempula commented Oct 4, 2020 •

edited

Loading

codecov bot commented Oct 4, 2020 •

edited

Loading

xiki-tempula Oct 5, 2020 •

edited

Loading