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

update analysis.base.AnalysisBase to conform to API #1463

Closed
orbeckst opened this issue Jul 10, 2017 · 11 comments
Closed

update analysis.base.AnalysisBase to conform to API #1463

orbeckst opened this issue Jul 10, 2017 · 11 comments
Labels
API Component-Analysis deprecation Deprecated functionality to give advance warning for API changes.
Milestone

Comments

@orbeckst
Copy link
Member

In #719 we decided on the details of the analysis interface ("Bauhaus" style). The description for implementation is on the wiki MDAnalysis.analysis user interface but we also decided to change a few additional things that are directly defined in the base class and automatically apply to all analysis code: The following needs to be changed in AnalysisBase:

  1. start, stop, step are kwargs of AnalysisBase.run() but not of __init__().
  2. verbose is a kwarg of __init__ where it sets the default; verbose is also a kwarg of run() where it overrides the default value for the duration of run (this allows the user to mute the frame-by-frame calculation but still have verbose output during pre- and post-processing).

Because 1 changes the API, we need to deprecate start, stop, step in __init__ and remove them in 1.0.0. Until then it needs to be possible to set the iteration range in __init__ and kwargs in run() will override the __init__ values.

@orbeckst
Copy link
Member Author

@kain88-de can you have a look and see how much work it is to get this done for 0.17.0?

@kain88-de
Copy link
Member

kain88-de commented Oct 27, 2017 via email

@richardjgowers richardjgowers modified the milestones: 0.17.0, 1.0 Jan 25, 2018
@orbeckst
Copy link
Member Author

orbeckst commented Jul 9, 2018

@richardjgowers can we get deprecations into 0.19.0 #1971 ?

In particular, the start/stop/step in run() is reversing a decision that we made much earlier when we tried to move them from run to __init__. We have depreciation code that says "start/stop/step will be removed from run() in 0.17.0" :-p.

@richardjgowers
Copy link
Member

@orbeckst I don't understand what needs doing for this, do some classes have start/stop/step in run?

@orbeckst
Copy link
Member Author

Classes should have start/stop/step in run() but not in __init__.

At least one (rms.RMSD.run()) has it already in run() but it is deprecated. It should be un-deprecated because it will become the norm.

@jbarnoud
Copy link
Contributor

jbarnoud commented Jul 10, 2018

Here are the things we need to do for this to come true:

  • Add start, stop, step, and verbose arguments to AnalysisBase.run.
  • Remove deprecated quiet from AnalysisBase.__init__.
  • Add deprecation warnings to AnalysisBase.__init__.
  • In analysis classes, move some logic from __init__ to _prepare.

bieniekmateusz added a commit to bieniekmateusz/mdanalysis that referenced this issue Jul 24, 2018
bieniekmateusz added a commit to bieniekmateusz/mdanalysis that referenced this issue Jul 28, 2018
@orbeckst orbeckst added the deprecation Deprecated functionality to give advance warning for API changes. label Aug 12, 2018
orbeckst added a commit that referenced this issue Sep 21, 2018
- Fixes #1975 
   removed quiet in favour of verbose
- Fixes #1979
   deprecated start/stop/step in AnalysisBase init
- Started work on AnalysisBase API update #1463
@orbeckst
Copy link
Member Author

@nawtrey added a parallel DensityAnalysis in MDAnalysis/pmda#93 which can be back-ported.

@IAlibay
Copy link
Member

IAlibay commented Feb 4, 2020

@orbeckst the removal of start, stop, and step from AnalysisBase.__init__() are next up on my list of things to do for #2443

As it is a bit unclear from the thread, I thought it might be worth asking if there is still work to be done here, or is it ready for removal?

@orbeckst
Copy link
Member Author

orbeckst commented Feb 4, 2020

  • I assume for anything using AnalysisBase, just removing will be fine. Just check the actual code of the analysis classes for any workarounds, deprecation messages, etc.
  • Any of the analysis classes that do not use AnalysisBase: keep them as they are as they do not fall under this issue. (If we can get any other classes more in line with the AnalysisBase API for 1.0 then that would be great — but raise separate issues and reference this issue.)

@lilyminium lilyminium mentioned this issue Jun 5, 2020
6 tasks
@IAlibay
Copy link
Member

IAlibay commented Jun 5, 2020

@orbeckst I think this was completed with #2505, is this good to close?

@orbeckst
Copy link
Member Author

orbeckst commented Jun 6, 2020

Yes, thanks for the reminder. This was done with #2505 and the changes to verbose (as part of one of the bigger gsoc PRs (EDIT: the tqdm/progressmeter one)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Component-Analysis deprecation Deprecated functionality to give advance warning for API changes.
Projects
None yet
Development

No branches or pull requests

5 participants