Use Results class for WaterBridge analysis#3287
Use Results class for WaterBridge analysis#3287orbeckst merged 96 commits intoMDAnalysis:developfrom
Conversation
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
- timeseries should become a result
- generate_tables() should return table (and for 2.x, continue storing it in .table) — not really related to results but now is the right time to make such a change
Otherwise looking good.
Additional eyes (especially @xiki-tempula and @IAlibay ) would be appreciated.
@PicoCentauri and @joaomcteixeira might want to take a look to see what kind of data structures are coming mdacli's way...
| @@ -1751,14 +1767,14 @@ def generate_table(self, output_format=None): | |||
| """Generate a normalised table of the results. | |||
|
|
|||
There was a problem hiding this comment.
add explanations
Parameters
----------
output_format : {'sele1_sele2', 'donor_acceptor'}
The output format of the `table` can be changed a fashion similar to
:attr:`WaterBridgeAnalysis.results.timeseries` by changing the labels
of the columns of the participating atoms.
Returns
-------
table : numpy.recarray
A "tidy" table with one hydrogen bond per row, labeled according to
`output_format` and containing information of atom_1, atom_2, distance,
and angle.There was a problem hiding this comment.
If there's an explanation of the format of table then it could go here, at least in a short version.
| @property | ||
| def table(self): | ||
| wmsg = ("The `table` attribute was deprecated in MDAnalysis 2.0.0 " | ||
| "and will be removed in MDAnalysis 3.0.0. Please use " | ||
| "`results.table` instead") | ||
| warnings.warn(wmsg, DeprecationWarning) | ||
| return self.results.table |
There was a problem hiding this comment.
remove — I don't think that we normally wrap attributes that are to be removed. Or do we, @IAlibay ?
Otherwise you need to store self.table in self._table and then use the property that you have here.
There was a problem hiding this comment.
I think I've missed part of the conversation here, what's the difference between this and say network below?
There was a problem hiding this comment.
Table will not be made a results.table. Instead we want to completely remove it for 3.0.0 (so that users will just use generate_table() to create it when they need it.
If you think we should issue a deprecation warning for the use of the attribute then we can do it. (I would then create self._results and have the table property just return that so that we don't trigger the warning when we set table in generate_table().)
Alternatively, we just put in a deprecation in the docs/CHANGELOG and be done.
There was a problem hiding this comment.
Alternatively, we just put in a deprecation in the docs/CHANGELOG and be done.
Sounds sensible to me.
xiki-tempula
left a comment
xiki-tempula
left a comment
There was a problem hiding this comment.
Thanks for the work. A few questions.
| @@ -1520,35 +1559,12 @@ def analysis(current, output, *args, **kwargs): | |||
|
|
|||
| timeseries = property(_generate_timeseries) | |||
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
|
Thank you @RMeli and @xiki-tempula . I updated and merged the CHANGELOG. We're now just waiting for CI. @IAlibay please feel free to merge if you get to it before I do. |
|
I checked the diff coverage https://app.codecov.io/gh/MDAnalysis/mdanalysis/compare/3287/diff and don't see a problem. Happy to merge. |
|
Thanks @orbeckst and @xiki-tempula for the insightful comments! |
5 participants
Fixes #3270
Changes made in this Pull Request:
results.networkresults.tableresults.timeseriesPR Checklist