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.

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

Adds Reader.timeseries #1400

Closed
wants to merge 4 commits into from
Closed

Adds Reader.timeseries #1400

wants to merge 4 commits into from

Conversation

richardjgowers
Copy link
Member

Fixes partially #1383

Changes made in this Pull Request:

  • added deprecation warnings to all core.Timeseries classes
  • added timeseries method to ProtoReader

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@richardjgowers richardjgowers added this to the 0.16.x milestone Jun 15, 2017
@richardjgowers
Copy link
Member Author

So Encore uses DCDReader.timeseries (which isn't actually a Timeseries (as in the class)) but instead just a handy method for iterating and grabbing coordinates. I've put a implementation of this into ProtoReader so that all readers get a basic version of it

@kain88-de kain88-de mentioned this pull request Jun 15, 2017
Closed
11 tasks
orbeckst
orbeckst requested changes Jun 16, 2017
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

The deprecations should be merged with #1403 (I can do that); the new trajectory.timeseries functionality is a new feature and has to go in 0.17.0.

package/MDAnalysis/coordinates/base.py
@@ -1461,6 +1461,60 @@ def __repr__(self):
natoms=self.n_atoms
))

def timeseries(self, asel=None, start=None, stop=None, step=None,
Copy link
Member

Choose a reason for hiding this comment

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

This is a new feature for trajectory readers. It cannot go into a patch release. It has to be in 0.17.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll bump this into 0.17 then

package/MDAnalysis/coordinates/DCD.py
@@ -578,6 +579,7 @@ def timeseries(self, asel=None, start=None, stop=None, step=None, skip=None,
# XXX needs to be implemented
return self._read_timeseries(atom_numbers, start, stop, step, format)

@deprecate(message="This method will be removed in 0.17")
Copy link
Member

Choose a reason for hiding this comment

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

All the deprecations can go in now.

I somehow missed your PR and did the reST deprecations in PR #1403

@orbeckst
Copy link
Member

@richardjgowers I cherry-picked your first two commits onto #1403

@richardjgowers richardjgowers modified the milestones: 0.17.0, 0.16.x Jun 16, 2017
@richardjgowers richardjgowers changed the title Issue 1383 timeseries deprecation Adds Reader.timeseries Jun 16, 2017
kain88-de
kain88-de requested changes Jun 24, 2017
View reviewed changes
package/CHANGELOG
@@ -17,6 +17,9 @@ mm/dd/yy

* 0.16.2

Deprecations
* deprecated core.Timeseries module (Issue #1383)
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be moved up the timeline or removed right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's leftover from when this also did the deprecation

package/MDAnalysis/coordinates/base.py
# do we need copy in this case?
coordinates[i] = ts.positions[atom_numbers].copy()

return coordinates
Copy link
Member

Choose a reason for hiding this comment

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

This is not implementing the format keyword argument. I guess you can copy the code I use in libdcd for this. There are also a lot of tests for it right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

And yeah, I'll have to deal with that in this PR too

Copy link
Member

@orbeckst orbeckst Jul 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

And we wanted to change the format kwarg to order.... #1400 (comment)

@kain88-de kain88-de mentioned this pull request Jun 25, 2017
Merged
4 tasks
kain88-de
kain88-de reviewed Jun 25, 2017
View reviewed changes
package/MDAnalysis/coordinates/base.py
step : int (optional)
Step size for reading; the default ``None`` is equivalent to 1 and means to
read every frame.
format : str (optional)
Copy link
Member

Choose a reason for hiding this comment

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

btw I'm for naming this option order format is confusing with the buildin

Copy link
Member

Choose a reason for hiding this comment

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

See #1063 where this was already mooted.

Copy link
Member

Choose a reason for hiding this comment

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

See #1453 for the issue.

@kain88-de kain88-de mentioned this pull request Jul 25, 2017
Closed
4 tasks
@richardjgowers richardjgowers modified the milestones: 0.17.0, 1.0 Jan 25, 2018
@richardjgowers richardjgowers mentioned this pull request Sep 25, 2018
Merged
4 tasks
@orbeckst orbeckst added the close? Evaluate if issue/PR is stale and can be closed. label Sep 28, 2018
orbeckst added a commit that referenced this pull request Jun 5, 2019
@orbeckst
timeseries for all readers (from PR #1400)
1913f73 
- updated format -> order kwarg
- updated docs
- note: general reader.timeseries only supports order="FAC" at the moment
- CHANGELOG
@orbeckst orbeckst mentioned this pull request Jun 5, 2019
Closed
4 tasks
@richardjgowers richardjgowers mentioned this pull request Mar 4, 2020
Closed
@hmacdope hmacdope mentioned this pull request Oct 26, 2022
Closed
@RMeli RMeli deleted the issue-1383-timeseries_deprecation branch December 23, 2023 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orbeckst orbeckst orbeckst requested changes

@kain88-de kain88-de kain88-de requested changes

Assignees
No one assigned
Labels
close? Evaluate if issue/PR is stale and can be closed. Work in progress
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@richardjgowers @orbeckst @kain88-de