Changed default memory layout of MemoryReader to 'fac' #1132
Conversation
|
Sorry for the delay. One small annoyance is that this code no longer works coordinates = universe.trajectory.timeseries(universe.atoms)
universe2 = mda.Universe(PSF, coordinates, format=MemoryReader)and has to be replaced with coordinates = universe.trajectory.timeseries(universe.atoms, format='fac')
universe2 = mda.Universe(PSF, coordinates, format=MemoryReader)or coordinates = universe.trajectory.timeseries(universe.atoms)
universe2 = mda.Universe(PSF, coordinates, format=MemoryReader, order='afc')Encore uses |
|
Perhaps this makes the case for a change of the timeseries return default to |
|
@mnmelo I agree about the intuition, although I'm a bit worried about how much user-code we would break by changing the default of timeseries. But at least changing the MemoryReader to the right layout now before release is a step in the right direction. |
| @@ -150,6 +150,6 @@ def get_ensemble_bootstrap_samples(ensemble, | |||
| size=ensemble.trajectory.timeseries().shape[1]) | |||
| ensembles.append( | |||
| mda.Universe(ensemble.filename, | |||
| ensemble.trajectory.timeseries(format='afc')[:,indices,:], | |||
| ensemble.trajectory.timeseries(format='fac')[indices,:,:], | |||
There was a problem hiding this comment.
you can now actually skip the last :, :] in the fancy index. Numpy will default to taking the full array for lower dimensions
There was a problem hiding this comment.
Great, thanks, @wouterboomsma !
Revisions:
- test that checks the default memory order
- optional: simplify the indexing by leaving out trailing
[..., :, :]
We can change timeseries at some other time. But at least by using format='fac' explicitly you will be future proof... (except that we will probably also rename format to order in timeseries() but that's a different issue).
| @@ -167,7 +161,7 @@ | |||
|
|
|||
| u2 = mda.Merge(protein).load_new( | |||
| AnalysisFromFunction(lambda ag: ag.positions.copy(), | |||
| protein).run().results.swapaxes(0, 1), | |||
There was a problem hiding this comment.
It's really nice that the swapaxes are gone now. That makes the code easier to understand
|
@wouterboomsma ping |
|
@kain88-de, @orbeckst I'm back. Sorry for the delay. I'll implement the changes requested by @orbeckst today or tomorrow. Two comments: I sometimes add the extra :,: to make it clear to the reader of the code how many dimensions the container has - which is not always clear from the context. But I can certainly remove this if you like. @orbeckst, when you write "test that checks the default memory order" do you mean tests that verify that after reading in a structure from file, the layout in the Memory Reader corresponds to the 'fac' standard. So for instance, reading it in with and with and without explicitly setting the format to 'fac' and then comparing that they are the same? |
|
Yes!
… Am Jan 12, 2017 um 7:40 schrieb Wouter Boomsma ***@***.***>:
@orbeckst, when you write "test that checks the default memory order" do you mean tests that verify that after reading in a structure from file, the layout in the Memory Reader corresponds to the 'fac' standard. So for instance, reading it in with and with and without explicitly setting the format to 'fac' and then comparing that they are the same?
|
For what it is worth, I prefer the explicit notation. |
|
Thanks. Will look later today. Really busy day.
…
|
|
All good, thank you. Can this be merged from your end? |
|
@orbeckst I don't think I have permissions to merge anything into the main repo (at least, I cannot see any "merge" button - could be due to my limited experience with github). |
|
No only maintainers can merge you haven't overlooked anything. |
Fixes #1063 and #1065
Changes made in this Pull Request:
PR Checklist