Skip to content

Split pathsimulator.py #754

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Jun 19, 2018
Merged

Split pathsimulator.py #754

merged 20 commits into from
Jun 19, 2018

Conversation

@dwhswenson
Copy link
Member

@dwhswenson dwhswenson commented Jan 28, 2018

As mentioned in #753, we should reorganize our file structure. I'm planning to do some work on parallelizing some path simulators soon, so I'm starting by reorganizing the pathsimulator.py file. Here's how I'm splitting it:

  • path_simulator.py: PathSimulator, MCStep (will move MCStep elsewhere later)
  • shoot_from_snapshot.py: ShootFromSnapshotsSimulation; CommittorSimulation
  • path_sampling.py: PathSampling
  • bootstrap_init_conds.py: all bootstrap stuff
  • direct_md.py: DirectSimulation

Note that the approach I've used to split involves some git acrobatics in order to preserve history on lines within the split files. See https://beyermatthias.de/blog/2014/09/24/splitting-files-while-preserving-history-in-git/.

@dwhswenson dwhswenson mentioned this pull request Jan 28, 2018
Open
4 tasks
@dwhswenson
Copy link
Member Author

dwhswenson commented Jan 28, 2018

Passes tests. Ready for review/merge.

@dwhswenson dwhswenson requested a review from jhprinz January 28, 2018 16:47
@dwhswenson dwhswenson mentioned this pull request Jan 30, 2018
Merged
6 tasks
@dwhswenson dwhswenson mentioned this pull request Apr 10, 2018
Closed
jhprinz
jhprinz approved these changes Jun 18, 2018
View reviewed changes
Copy link
Contributor

@jhprinz jhprinz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. Make it much cleaner

@jhprinz jhprinz assigned dwhswenson and unassigned jhprinz Jun 18, 2018
@dwhswenson
Copy link
Member Author

dwhswenson commented Jun 19, 2018

Thanks -- I have some rough plans for doing the same with ensemble.py and pathmover.py, and will do those later. Will merge this one now!

@dwhswenson dwhswenson merged commit 04a7754 into openpathsampling:master Jun 19, 2018
@sroet
Copy link
Member

sroet commented Jun 20, 2018

I would have liked an API Break or module rename label on this PR.
It broke the import of pathsimulator by the renaming to pathsimulators. This change is also not mentioned in the comments.

@dwhswenson
Copy link
Member Author

dwhswenson commented Jun 20, 2018

What were you importing from openpathsampling.pathsimulator? Isn't everything that is public also directly importable from the main openpathsampling namespace?

In order to make sure everything only shows up in only one place in the (official, public) API, I think that only the least-nested level should be used. For most of OPS, that means the root openpathsampling namespace.

This was referenced Jun 20, 2018
Merged
Merged
@sroet
Copy link
Member

sroet commented Jun 21, 2018

@dwhswenson

What were you importing from openpathsampling.pathsimulator? Isn't everything that is public also directly importable from the main openpathsampling namespace?

You are correct, thanks for the tip. This came up in one of my old E-CAM modules (the transition state ensemble). It is now fixed, and imports from the openpathsampling namespace.

@dwhswenson dwhswenson deleted the separate_pathsimulator branch January 21, 2019 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jhprinz jhprinz jhprinz approved these changes

Assignees

@dwhswenson dwhswenson

Labels

misc PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dwhswenson @sroet @jhprinz