Skip to content

Adds Reader.timeseries#1400

Closed
richardjgowers wants to merge 4 commits intodevelopfrom
issue-1383-timeseries_deprecation
Closed

Adds Reader.timeseries#1400
richardjgowers wants to merge 4 commits intodevelopfrom
issue-1383-timeseries_deprecation

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
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
11 tasks
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.

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

# 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
* 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

# 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

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
4 tasks
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.

@richardjgowers richardjgowers modified the milestones: 0.17.0, 1.0 Jan 25, 2018
@richardjgowers richardjgowers mentioned this pull request Sep 25, 2018
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
- 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
4 tasks
@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

Labels

close? Evaluate if issue/PR is stale and can be closed. Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants