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

transfer_to_memory doesn't copy dynamics dimension information #1041

Closed
kain88-de opened this issue Oct 20, 2016 · 9 comments
Closed

transfer_to_memory doesn't copy dynamics dimension information #1041

kain88-de opened this issue Oct 20, 2016 · 9 comments

Comments

@kain88-de
Copy link
Member

Expected behaviour

If I load a trajectory from an NPT simulation into memory I would expect that the memory reader also copies the dimension information for every frame

Actual behaviour

Only the information from the first frame is used.

@orbeckst
Copy link
Member

I consider this a bug and because ts.dimensions is a top level attribute of Timestep it should also be copied.

However, it raises the question if the MemoryReader should also be aware of other per-timestep information – aren't we storing these data nowadays in a ts.data dict?

@kain88-de
Copy link
Member Author

The data dict is reader specific. I'm OK if that isn't copied. But other available top level attributes should be copied if available.

@jbarnoud
Copy link
Contributor

Do we have an equivalent of Reader.timeseries for the box shape? Fixing this is not difficult, but I have to iterate through the trajectory even when the faster timeseries is available.

@kain88-de
Copy link
Member Author

We don't have an equivalent. But libdcd reader does return it as well. I wouldn't mind adding a method that can also read the box information for all frames and returns them as an array. We should think if we want to add this to timeseries though to avoid iterating several times over a possibly large trajectory.

@orbeckst
Copy link
Member

orbeckst commented Jul 16, 2017 via email

@jbarnoud
Copy link
Contributor

If libdcd has it, then it would make sense to expose it. Not much of a priority, but convenient at least in that context.

@kain88-de
Copy link
Member Author

Btw xarray sounds like the right package for a data format that such a universal time series should return.

@orbeckst
Copy link
Member

orbeckst commented Aug 14, 2017

@jbarnoud if you fix this issue then we will have dealt with all defect for 0.17 issues.

EDIT: use future perfect tense when you legitimately can!

@orbeckst
Copy link
Member

orbeckst commented Dec 9, 2017

I'd really like this one to work... but it looks as if the chances for this getting fixed in 0.17.0 are slim. It is a bug fix so this can always go into a 0.17.x.

Feeback from anyone working on this issue would be welcome.

@orbeckst orbeckst added the PBC label Jan 24, 2018
@richardjgowers richardjgowers modified the milestones: 0.17.0, 1.0 Jan 25, 2018
richardjgowers added a commit that referenced this issue Oct 1, 2018
Universe.empty now creates a MemoryReader trajectory

Removed DummyReader (it was buggy)

added varying box size to MemoryReader

Universe.transfer_to_memory now also transfer velocities and forces

MemoryReader now always has array of dimensions

fixes Issue #2081 #2076 #2077 #1041
richardjgowers added a commit that referenced this issue Oct 9, 2018
Universe.empty now creates a MemoryReader trajectory

Removed DummyReader (it was buggy)

added varying box size to MemoryReader

Universe.transfer_to_memory now also transfer velocities and forces

MemoryReader now always has array of dimensions

fixes Issue #2081 #2076 #2077 #1041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants