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

Remove TimeSeries #1373

Closed
kain88-de opened this issue Jun 1, 2017 · 12 comments
Closed

Remove TimeSeries #1373

kain88-de opened this issue Jun 1, 2017 · 12 comments
Assignees
@kain88-de
Labels
API Component-Core Format-DCD maintainability
Milestone
0.17.0

Comments

@kain88-de
Copy link
Member

We discussed recently to remove the TimeSeries objects from MDAnalysis. As far as I know none of the core devs is using it. We also note that it is unlikely we will ever implement TimeSeries for any other readers besides dcd and memory and most of it's function can be replaced by the Analysis Framework.

The only part in the library that uses TimeSeries is encore. So it would be useful get some input from @wouterboomsma and @mtiberti before we go ahead with this.

As a first step I would remove timeseries from DCD during the cython transition.
In a later step we would remove timeseries from memory reader and remove the core TimeSeries class.
@richardjgowers richardjgowers added this to the 1.0 milestone Jun 1, 2017
@kain88-de
Copy link
Member Author

I should be more specific. This won't remove the timeseries call from MemoryReader. As this is only a view of the underlying array (useful to have). Only the fancy correl functions will be removed.

@wouterboomsma
Copy link
Contributor

As far as I remember, the only reason Encore uses timeseries() was to maximize it's similarity to other Readers. So I don't think there should be any problems removing it. However, I think it would make sense to keep the functionality (perhaps with a different name) within MemoryReader, because it is an intuitive interface to extract a numpy array in a specified format.

@wouterboomsma
Copy link
Contributor

Ups... Posted before seeing your follow-up message. It seems we agree.

@wouterboomsma
Copy link
Contributor

If we keep the timeseries() method in MemoryReader, your suggested change would probably not require any modifications in Encore at all - because encore typically works on MemoryReader readers anyway (calling transfer_to_memory if necessary).

@orbeckst orbeckst mentioned this issue Jun 1, 2017
Merged
4 tasks
@orbeckst
Copy link
Member

orbeckst commented Jun 1, 2017

This would also close #186.

@kain88-de
Copy link
Member Author

So there is a general agreement to remove the correl like features?

@kain88-de kain88-de mentioned this issue Jun 2, 2017
Merged
4 tasks
@orbeckst orbeckst removed the proposal label Jun 2, 2017
@orbeckst
Copy link
Member

orbeckst commented Jun 2, 2017

Yes, I think everyone would like to move forward with removing it. I removed the proposal tag.

@richardjgowers
Copy link
Member

Ok, once API breaks are decided, we should really put the deprecation notices in asap. It can even go in a "bugfix" release right? 0.17 will be soonish (tm), so maybe target this (and similar API changes) for 0.18? And hopefully 0.18 == 1.0?

@orbeckst
Copy link
Member

orbeckst commented Jun 4, 2017 via email

@kain88-de
Copy link
Member Author

This change has to come with the DCD update. I could try to make it work but I rather not. It's a hard cut admittedly but there is nothing else using the TimeSeries stuff.

@richardjgowers
Copy link
Member

Ok, so for 0.17 == py3, we need also for this issue to be 0.17? I don't think anyone is using this, so maybe we can do that

@richardjgowers richardjgowers modified the milestones: 0.17.0, 1.0 Jun 4, 2017
@richardjgowers richardjgowers mentioned this issue Jun 4, 2017
Closed
3 tasks
@orbeckst orbeckst mentioned this issue Jun 12, 2017
Closed
@orbeckst
Copy link
Member

I added a longer comment to #1383 basically arguing that we should deprecate Timeseries/correl in 0.16.2 and remove in 0.17.0.

orbeckst added a commit that referenced this issue Jun 15, 2017
@orbeckst
add deprecation warnings for API changes
088d309 
- closes #1383
- included deprecations:
  - #1373 Timeseries (targeted for 0.17)
    Note that the deprecation for core.Timeseries will always show up;
    this is deliberate so that users WILL see it as it will be gone in
    the next release!
  - #1377 Quick selectors (target 1.0)
  - #782 flags (target 1.0)
- updated CHANGELOG
@orbeckst orbeckst mentioned this issue Jun 15, 2017
Merged
4 tasks
kain88-de pushed a commit that referenced this issue Jun 20, 2017
@orbeckst @kain88-de
add deprecation warnings for API changes
649cf89 
- closes #1383
- included deprecations:
  - #1373 Timeseries (targeted for 0.17)
    Note that the deprecation for core.Timeseries will always show up;
    this is deliberate so that users WILL see it as it will be gone in
    the next release!
  - #1377 Quick selectors (target 1.0)
  - #782 flags (target 1.0)
- updated CHANGELOG
richardjgowers pushed a commit that referenced this issue Jun 21, 2017
@orbeckst @richardjgowers
add deprecation warnings for API changes
0a37e9e 
- closes #1383
- included deprecations:
  - #1373 Timeseries (targeted for 0.17)
    Note that the deprecation for core.Timeseries will always show up;
    this is deliberate so that users WILL see it as it will be gone in
    the next release!
  - #1377 Quick selectors (target 1.0)
  - #782 flags (target 1.0)
- updated CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kain88-de kain88-de

Labels
API Component-Core Format-DCD maintainability
Projects
None yet
Milestone
0.17.0
Development

No branches or pull requests
4 participants
@orbeckst @wouterboomsma @kain88-de @richardjgowers