Time convergence #168

xiki-tempula · 2021-09-26T11:12:03Z

As discussed in #159, the time convergence analysis could be implemented as a function.

codecov · 2021-09-26T11:14:56Z

Codecov Report

Merging #168 (f42dcde) into master (5f81d70) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   97.85%   97.96%   +0.10%     
==========================================
  Files          20       22       +2     
  Lines        1214     1279      +65     
  Branches      258      271      +13     
==========================================
+ Hits         1188     1253      +65     
  Misses          5        5              
  Partials       21       21

Impacted Files	Coverage Δ
src/alchemlyb/convergence/__init__.py	`100.00% <100.00%> (ø)`
src/alchemlyb/convergence/convergence.py	`100.00% <100.00%> (ø)`
src/alchemlyb/visualisation/convergence.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 5f81d70...f42dcde. Read the comment docs.

orbeckst

Good to have these first convergence functions. However, they should be in their own module alchemlyb.convergence as they are not postprocessing, see https://alchemlyb.readthedocs.io/en/latest/api_principles.html.

Please

create new top level convergence module
include in docs
update API principles with short description of new module
update CHANGES

I'd also think the convergence results dataframe could be improved (see comments).

Finally, if you change the argument processing of the plot_convergence function then you don't have to effectively change its API.

Please see inline for any other comments.

src/alchemlyb/postprocessors/convergence.py

orbeckst · 2021-10-04T08:01:33Z

src/alchemlyb/postprocessors/convergence.py

+    forward_list = []
+    forward_error_list = []
+    for i in range(1, num + 1):
+        logger.info('Forward analysis: {:.2f}%'.format(i / num))


fraction of data i/num should be in final dataframe

thanks for adding, please see new comments about making it a float

src/alchemlyb/visualisation/convergence.py

orbeckst · 2021-10-05T06:58:47Z

src/alchemlyb/convergence/convergence.py

+
+    .. versionadded:: 0.6.0
+    '''
+    logger = logging.getLogger('alchemlyb.postprocessors.'


convergence

src/alchemlyb/convergence/convergence.py

orbeckst · 2021-10-05T07:03:10Z

src/alchemlyb/visualisation/convergence.py

-        A list of error from the last X% of data.
+    data : Dataframe or 4 Lists
+        Output Dataframe from
+        :func:`~alchemlyb.postprocessors.convergence.forward_backward_convergence`.


convergence

orbeckst · 2021-10-05T07:05:12Z

src/alchemlyb/visualisation/convergence.py

-        A list of free energy estimate from the last X% of data.
-    backward_error : List
-        A list of error from the last X% of data.
+    data : Dataframe or 4 Lists


are these really lists, maybe better array_like

src/alchemlyb/visualisation/convergence.py

orbeckst · 2021-10-05T07:10:31Z

src/alchemlyb/visualisation/convergence.py

@@ -32,12 +31,28 @@ def plot_convergence(forward, forward_error, backward, backward_error,
    The code is taken and modified from
    `Alchemical Analysis <https://github.com/MobleyLab/alchemical-analysis>`_.

-    The units variable is for labelling only. Changing it doesn't change the
-    unit of the underlying variable.
+    If `dataframe` is not provide, the units variable is for labelling only.


If `data` is not an :class:pandas.Dataframe` produced by :func:`.....forward_backward_convergence` ...

orbeckst · 2021-10-05T07:12:22Z

src/alchemlyb/visualisation/convergence.py

+    if len(data) == 1 and isinstance(data[0], pd.DataFrame):
+        dataframe = get_unit_converter(units)(data)
+        forward = dataframe['Forward'].to_numpy()
+        forward_error = dataframe['F. Error'].to_numpy()


How could "F. Error" pass the tests, given that you changed the column names?

orbeckst · 2021-10-05T07:14:18Z

src/alchemlyb/convergence/convergence.py

+    forward_list = []
+    forward_error_list = []
+    for i in range(1, num + 1):
+        logger.info('Forward analysis: {:.2f}%'.format(i / num))


Multiply by 100 for %.

orbeckst · 2021-10-05T07:14:39Z

src/alchemlyb/convergence/convergence.py

+    backward_list = []
+    backward_error_list = []
+    for i in range(1, num + 1):
+        logger.info('Backward analysis: {:.2f}%'.format(i / num))


x 100 for %

orbeckst · 2021-10-05T07:17:53Z

src/alchemlyb/convergence/convergence.py

+         'Forward_Error': forward_error_list,
+         'Backward': backward_list,
+         'Backward_Error': backward_error_list},
+        index=['{}/{}'.format(i, num) for i in range(1, num + 1)])


I prefer a numerical column instead of a string. If you want to keep it as the index, also add a column data_fraction where you just store the float i/num. The numerical value is more valuable, I think, because you can directly plot against it. (Or replace the string with the float and keep it the rest as is.)

I realize my original comment might have been misleading where I talked about i/num, hope the above makes my thinking clearer.

orbeckst · 2021-10-05T07:24:16Z

Thanks for addressing my comments. I have a bunch of additional comments added (and still needs CHANGES entry). Thanks!

orbeckst · 2021-10-05T22:41:07Z

Tests fail with something that looks like passing through a list or array when a dataframe is expected.

xiki-tempula · 2021-10-07T19:36:04Z

@orbeckst Thank you for the review. Sorry I was busy in the last few days.

orbeckst

Please see change suggestions (you should be able to just apply them) and the remaining docs issue. Thanks!

orbeckst · 2021-10-10T07:31:45Z

src/alchemlyb/visualisation/convergence.py

-    If `dataframe` is not provide, the units variable is for labelling only.
+    If `data` is not an :class:pandas.Dataframe` produced by
+    :func:`~alchemlyb.convergence.forward_backward_convergence`,
+    the unit will be adjusted accoridng to the units


Suggested change

the unit will be adjusted accoridng to the units

the unit will be adjusted according to the units

src/alchemlyb/tests/test_convergence.py

CHANGES

src/alchemlyb/visualisation/convergence.py

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

xiki-tempula · 2021-10-22T08:22:55Z

@orbeckst Sorry, I wonder if you would mind giving a final review and I will get it merged then.

orbeckst · 2021-10-22T15:59:44Z

@xiki-tempula I was working on the commit message, which is now lost because you just merged — that's why I had assigned myself to indicate that I would be merging it. Let's agree that before merging we claim the Assignee to avoid confusion.

xiki-tempula · 2021-10-22T16:02:07Z

@orbeckst I'm very sorry. I saw that it has been approved so I just went ahead and merged it. If I saw it has been assigned in the future, I will not touch it.

xiki-tempula added 4 commits September 25, 2021 20:53

Create convergence.py

cd2ff92

Update convergence.py

aa3054e

Update convergence.py

ef21b2d

update

7238298

xiki-tempula marked this pull request as draft September 26, 2021 11:13

xiki-tempula added 2 commits September 26, 2021 12:40

update

8ad6bb2

update doc

e87867f

xiki-tempula marked this pull request as ready for review September 26, 2021 12:43

update doc

7d2b936

xiki-tempula requested a review from orbeckst September 26, 2021 12:52

orbeckst requested changes Oct 4, 2021

View reviewed changes

xiki-tempula added 2 commits October 4, 2021 21:25

update

36f9adc

update

f9d08bc

orbeckst reviewed Oct 5, 2021

View reviewed changes

src/alchemlyb/convergence/convergence.py Show resolved Hide resolved

orbeckst reviewed Oct 5, 2021

View reviewed changes

src/alchemlyb/visualisation/convergence.py Show resolved Hide resolved

orbeckst reviewed Oct 5, 2021

View reviewed changes

Merge branch 'master' into convergence

daa9870

xiki-tempula and others added 3 commits October 7, 2021 20:09

update

305ceac

change parser

212e955

Update test_convergence.py

d281582

xiki-tempula added 2 commits October 7, 2021 20:24

update

f6ce666

Update convergence.py

e9601fb

xiki-tempula added 2 commits October 7, 2021 20:38

Update convergence.py

a4d379a

Update convergence.py

1340367

orbeckst requested changes Oct 10, 2021

View reviewed changes

xiki-tempula and others added 3 commits October 10, 2021 09:03

Apply suggestions from code review

b58db4a

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

Update convergence.py

afc8d68

Update convergence.py

a0ade56

xiki-tempula requested a review from orbeckst October 10, 2021 08:26

Merge branch 'master' into convergence

f42dcde

orbeckst approved these changes Oct 22, 2021

View reviewed changes

orbeckst self-assigned this Oct 22, 2021

xiki-tempula merged commit 592f38b into alchemistry:master Oct 22, 2021

xiki-tempula deleted the convergence branch October 22, 2021 15:55

orbeckst mentioned this pull request Oct 29, 2021

release 0.6.0 #174

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time convergence #168

Time convergence #168

xiki-tempula commented Sep 26, 2021

codecov bot commented Sep 26, 2021 •

edited

Loading

orbeckst left a comment

orbeckst Oct 4, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021

orbeckst Oct 5, 2021 •

edited

Loading

orbeckst Oct 5, 2021

orbeckst commented Oct 5, 2021

orbeckst commented Oct 5, 2021

xiki-tempula commented Oct 7, 2021

orbeckst left a comment

orbeckst Oct 10, 2021

xiki-tempula commented Oct 22, 2021

orbeckst commented Oct 22, 2021

xiki-tempula commented Oct 22, 2021 •

edited

Loading

	the unit will be adjusted accoridng to the units
	the unit will be adjusted according to the units

Time convergence #168

Time convergence #168

Conversation

xiki-tempula commented Sep 26, 2021

codecov bot commented Sep 26, 2021 • edited Loading

Codecov Report

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

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 Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbeckst commented Oct 5, 2021

orbeckst commented Oct 5, 2021

xiki-tempula commented Oct 7, 2021

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiki-tempula commented Oct 22, 2021

orbeckst commented Oct 22, 2021

xiki-tempula commented Oct 22, 2021 • edited Loading

codecov bot commented Sep 26, 2021 •

edited

Loading

orbeckst Oct 5, 2021 •

edited

Loading

xiki-tempula commented Oct 22, 2021 •

edited

Loading