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

change timeseries "format" kw to "order" #1453

Closed
orbeckst opened this issue Jul 6, 2017 · 9 comments · Fixed by #1751
Closed

change timeseries "format" kw to "order" #1453

orbeckst opened this issue Jul 6, 2017 · 9 comments · Fixed by #1751
Assignees
Labels
Milestone

Comments

@orbeckst
Copy link
Member

orbeckst commented Jul 6, 2017

In various issues and discussions we sort-of decided that the Reader.timeseries() method should not use format="afc" as a kwarg but order="afc" (which make it consistent with the MemoryReader, avoids clashes with the builtin format, ... and avoids clashes with the format kwarg of Universe (yes, that one clashes with the builtin, too)).

@orbeckst orbeckst added the API label Jul 6, 2017
This was referenced Jul 6, 2017
@orbeckst
Copy link
Member Author

I think we should consider adding this to 0.17 so that we have some deprecation time.

@orbeckst orbeckst added this to the 0.17.0 milestone Jul 21, 2017
@richardjgowers richardjgowers mentioned this issue Jul 21, 2017
6 tasks
@orbeckst
Copy link
Member Author

orbeckst commented Aug 21, 2017

Note to myself: While playing around with the memory reader for the Examples for RMSF I found that not all orderings (keyword order=...) work for Universe.load_new(array_like, order="..."). At least I got a cryptic error with fac (something that there were too few dimensions) when I tried a array_like for a single frame. It worked with afc, which I put explicitly in the example (but it is also the default for e.g. DCReader.timeseries).

Need to investigate more and possibly add tests for all permutations when reading with the MemoryReader.

@orbeckst
Copy link
Member Author

Also note that the docs for the MemoryReader need to be update:

  • When a array_like is provided then it is automatically passed to the MemoryReader and the format="MEMORY" or format=MemoryReader is not needed anymore.
  • Same for Universe.load_new()

In general we need to clarify how file types are recognized.

@orbeckst orbeckst self-assigned this Oct 27, 2017
@orbeckst
Copy link
Member Author

I'll try to do this in my ample spare time ;-) – unless anyone else wants to: feel free to snatch this issue from me.

@kain88-de
Copy link
Member

@orbeckst do you have any code for your experiments of order that you did in august.

@orbeckst
Copy link
Member Author

orbeckst commented Jan 2, 2018

No, sorry. But you can take the Example at https://www.mdanalysis.org/mdanalysis/documentation_pages/analysis/rms.html#MDAnalysis.analysis.rms.RMSF and just change order in the line

# make a reference structure (need to reshape into a 1-frame "trajectory")
reference = mda.Merge(protein).load_new(
            reference_coordinates[:, None, :], order="afc")

and it should fail.

(I am travelling and if you want to work on this please by all means do so and also assign yourself/un-assign me. Thanks!)

@kain88-de
Copy link
Member

Your example case works now with my changes.

@orbeckst
Copy link
Member Author

orbeckst commented Jan 5, 2018

@kain88-de many thanks for finishing the PR, I very much appreciate you jumping in. Sorry I wasn't able to be more helpful.

@kain88-de
Copy link
Member

kain88-de commented Jan 5, 2018 via email

orbeckst pushed a commit that referenced this issue Feb 29, 2020
* Completes #1453 for MemoryReader (and towards v1.0 #2443)
* Removes support for the deprecated format keyword in MemoryReader.timeseries.
* Removes deprecation test
* Update CHANGELOG
lilyminium pushed a commit to lilyminium/mdanalysis that referenced this issue Mar 4, 2020
* Completes MDAnalysis#1453 for MemoryReader (and towards v1.0 MDAnalysis#2443)
* Removes support for the deprecated format keyword in MemoryReader.timeseries.
* Removes deprecation test
* Update CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants