Time convergence #168
Time convergence #168
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
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.
| forward_list = [] | ||
| forward_error_list = [] | ||
| for i in range(1, num + 1): | ||
| logger.info('Forward analysis: {:.2f}%'.format(i / num)) |
There was a problem hiding this comment.
fraction of data i/num should be in final dataframe
There was a problem hiding this comment.
thanks for adding, please see new comments about making it a float
|
|
||
| .. versionadded:: 0.6.0 | ||
| ''' | ||
| logger = logging.getLogger('alchemlyb.postprocessors.' |
| 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`. |
| 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 |
There was a problem hiding this comment.
are these really lists, maybe better array_like
| @@ -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. | |||
There was a problem hiding this comment.
If `data` is not an :class:pandas.Dataframe` produced by :func:`.....forward_backward_convergence` ...
| 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() |
There was a problem hiding this comment.
How could "F. Error" pass the tests, given that you changed the column names?
| forward_list = [] | ||
| forward_error_list = [] | ||
| for i in range(1, num + 1): | ||
| logger.info('Forward analysis: {:.2f}%'.format(i / num)) |
| backward_list = [] | ||
| backward_error_list = [] | ||
| for i in range(1, num + 1): | ||
| logger.info('Backward analysis: {:.2f}%'.format(i / num)) |
| 'Forward_Error': forward_error_list, | ||
| 'Backward': backward_list, | ||
| 'Backward_Error': backward_error_list}, | ||
| index=['{}/{}'.format(i, num) for i in range(1, num + 1)]) |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
I realize my original comment might have been misleading where I talked about i/num, hope the above makes my thinking clearer.
|
Thanks for addressing my comments. I have a bunch of additional comments added (and still needs CHANGES entry). Thanks! |
|
Tests fail with something that looks like passing through a list or array when a dataframe is expected. |
|
@orbeckst Thank you for the review. Sorry I was busy in the last few days. |
| 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 |
There was a problem hiding this comment.
| the unit will be adjusted accoridng to the units | |
| the unit will be adjusted according to the units |
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
|
@orbeckst Sorry, I wonder if you would mind giving a final review and I will get it merged then. |
|
@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. |
|
@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. |
As discussed in #159, the time convergence analysis could be implemented as a function.