Add packages as a space for user contributions#849
Conversation
This is not the canonical location, and it is still available at openapthsampling.VisitAllStatesEnsemble, so no API break.
|
This is ready for review. See update of the top post for a more detailed description. Since this is significant for future development (it is more an "enhancement proposal" than a new feature), I'd like an actual review on this, instead of just waiting for a period of comment. @jhprinz, please look this over if you can. Also, it would be nice to get input from potential future contributors: @sroet and @hejung, if either of you have a chance, I'd appreciate your input. I won't merge until there's at least one review of this. |
sroet
left a comment
There was a problem hiding this comment.
I am missing a subpackage for the shootingpoint selectors and modifiers?
LGTM otherwise.
I think these can go directly in the In any case, even if we move things out of the main Final point: I definitely think that, unless the resulting file reaches ~1000 lines, it is best to put the shooting point selectors/snapshot modifiers specific to a new mover in a single file with the new mover. Again, this goes to the idea that a single file with all the code will be easier for a new developer to understand. This just means that the import statement in |
Yes, I think so.
Sounds reasonable, I was just wondering about them, as those classes were not mentioned in the pathmovers/README.md (like |
Added a parenthetical about "helper classes" for your pathmover. Hope that clears it up! |
hejung
left a comment
There was a problem hiding this comment.
If I understand correctly all code related to collectivevariables (i.e. the content of collectivevariable.py) will at some point end up in cv_wrappers? In this case I would suggest to rename cv_wrappers to collectivevariables for consistency reasons.
Otherwise looks good to me too.
It does, thank you. |
Followed this suggestion. In general, we have consistency problems with CVs. Do we call it |
In the near future, (I hope) we will have a number of user contributions in several areas, including ensembles, path movers, and CV wrappers. I made a few empty subpackages that will be a good place to keep those. Eventually, we should also migrate our existing versions of these things into those subpackages.
UPDATE: I went ahead and moved my
VisitAllStatesEnsembleinto theensemblessubpackage, so that gives a first example of how this will be done.In detail, here's what I've implemented:
This PR is basically saying that we'll use something like what was suggested in #753 for future contributions. In the long run, I'd like for most of our existing code to be restructured into this format as well (as said in #753).
New ensembles, CV wrappers, and path movers will go into special subpackages for those. The idea is that everything related to adding one of these will usually fit in a single file, and this will help people browsing the source find the thing they're interested in. We already do this (to some extent) for engines and for path simulators.
By managing the correct imports in the subpackage, so that, e.g.,
ensembles/__init__.pyonly includes things that should be public-facing, we can safelyfrom .ensembles import *in our main__init__.py, and the root namespace has all the public-facing stuff.For ensembles and CVs, this is essentially saying that our huge single-file modules for each should be a package. That makes sense. For path movers, I'm suggesting something more significantly different from what we currently do. Path movers usually have a move strategy associated with them, and sometimes also have a special move scheme. I'm suggesting that users put path mover, strategy, and scheme all in a single file. This will be useful for new developers who want to create something similar to what already exists -- a complete example in one file is better than bits of code scattered across several files.
In these cases, the developer will then import the mover into
pathmovers/__init__.py, while the scheme and strategy will be imported intopathmovers/move_schemes.pyandpathmovers/move_strategies.py, respectively. Again, each of these intermediate imports serves as the gatekeeper for the public API, so we can do afrom foo import *in the main__init__.py.Previously
openpathsampling.strategieshad basically been an alias ofopenpathsampling.high_level.move_strategy. Now I've made it into a proper subpackage that contains everything fromhigh_level.move_strategyas well aspathmovers.move_strategies.