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

Restructuring of Snapshots #429

Merged
merged 68 commits into from Mar 3, 2016
Merged

Conversation

jhprinz
Copy link
Contributor

@jhprinz jhprinz commented Jan 22, 2016

This will

  • precompile feature-dependant code
  • store snapshots more compact (no overhead for reversed objects)
  • fixes CVs
  • remove energies from snapshots (to be turned into a CV)

do be done:

  • better docstring generation from features
  • cleanup of CVs that will use compressed storage now. The code is not optimal.
  • implement a renew saving function that allows to reuse Snapshots
  • check docstrings
  • get existing tests pass
  • fix notebooks

most stuff as discussed with @dwhswenson.

To me merged after #412 .

This makes the use of features much easier. And the actual implementation of Snapshot now looks like

@features.base.set_features(
    features.configuration,
    features.momentum,
    features.xyz,
    features.topology  # for compatibility
)
class Snapshot(FeatureSnapshot):
    """
    A fast MDSnapshot
    """

The decorator takes care of everything. It will construct the constructor with correct signature. Precompile a .copy() and .create_reversed() functions, and take care of docstrings, etc...

The xyz feature just add a property, nothing else.

A pair of forward and backward Snapshot will now only be stored once. Never twice. Storage takes care of getting the reversed and each feature much now, if it reverses or not. This will make storage requirements smaller. Same is true for CVs which will now consume half as much space if time-reversible.

The special saving should do the following step in one: 1. Save the snapshot 2. Do not cache it and 3. Take care of potential references in the reversed.

This also implements storing the topology with each Snapshot as a feature that can be later removed easily when switching to storing the engine.

I decided to refactor the code for loading and saving CVs while working on better caching since this is kind of the same problem.

Last things related to this:

  • make energies into an optional CV (already implemented)
  • engine restructuring into different folders (needs to be discussed, but simple)
  • Snapshots use engine property and not topology (simple)
  • Question if engines use their own snapshot stored, which can be shared
  • Split caching of objects into separate function. Should be reusable for objectstore and CVs and allow chunked loading (optional)

@jhprinz jhprinz changed the title Restructuring of Snapshots [WIP] Restructuring of Snapshots Jan 22, 2016
@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 1, 2016

using mdtraj version 1.5.1 for now...

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 1, 2016

Seems that ToyDynamics is faster now.

@dwhswenson
Copy link
Member

Seems that ToyDynamics is faster now.

Any idea what makes the difference? I was concerned that the flexibility of the new snapshot style would slow things down, not speed them up.

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 1, 2016

This is because I cleaned and improved the code so that creation and reversal is simpler and thus faster.

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 1, 2016

I will check this out little more.

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 1, 2016

Back on track. All tests pass...

I think we are almost ready to merge...

@dwhswenson
Copy link
Member

The only unresolved issue I see is the renaming of Configuration and Momentum to StaticContainer/KineticContainer (or StaticsContainer, if you prefer). That could be done in a separate PR, but it should be pretty easy -- can we go ahead and do that in here?

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 2, 2016

Yes, I would put this into the engine restructuring. Once this is really separate we can easily change the name of the containers.

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 2, 2016

Let me run a few more tests. But I think we should be good to go.

@dwhswenson
Copy link
Member

Let me run a few more tests. But I think we should be good to go.

Should I still be expecting more tests? Otherwise, I'm ready to merge this (on the assumption that the renaming still to do happens in #434, or at least very soon.)

@jhprinz
Copy link
Contributor Author

jhprinz commented Mar 3, 2016

Good to go. Please merge if you want to.

@jhprinz jhprinz changed the title [WIP] Restructuring of Snapshots Restructuring of Snapshots Mar 3, 2016
@jhprinz jhprinz assigned jhprinz and dwhswenson and unassigned jhprinz Mar 3, 2016
@dwhswenson
Copy link
Member

Oops! Meant to merge this as soon as I got back from the IND (where I did not get my residence permit this time... have to go back in a week or two). Anyway -- Merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants