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

Reader API cleanup #350

Closed
richardjgowers opened this issue Jul 14, 2015 · 12 comments
Closed

Reader API cleanup #350

richardjgowers opened this issue Jul 14, 2015 · 12 comments

Comments

@richardjgowers
Copy link
Member

So I'm going through the Reader API (for Issue #306) and it probably needs a spring clean.

"Required" attributes that I'm going to ditch:

  • fixed (DCD specific, move it into DCD only)
  • skip (step size of iterating through the trajectory? remove)
  • skip_timestep (number of steps between saving the trajectory, move to optional)
  • delta (md integrator timestep, move to optional)
  • periodic (if the trajectory has box information, remove)
  • dt (time between frames, moved to Timestep)
@orbeckst
Copy link
Member

Can we create a dict metadata or data or whatever (similar to Timestep.data) that keeps all the specific information that one might want to know in specific cases but that is not essential for the API? Basically anything labelled "optional" would go there.

The two attributes that I have some more thoughts on are

  • periodic
    • make optional
    • but keep around because some readers provide this information
  • dt

So basically, I think dt is pretty fundamental at the Reader level, too, and I think it makes sense to keep it as Reader.dt.

Opinions?

@richardjgowers
Copy link
Member Author

Hmm ok, periodic gets a stay of execution.

And yeah, dt is going to be a bit more involved to get everything correct, but not impossible.

@orbeckst
Copy link
Member

On 16 Jul, 2015, at 11:02, Richard Gowers wrote:

Hmm ok, periodic gets a stay of execution.

We don't really use it anywhere else, I think (maybe to decide what to do with unitcell), so just dropping it into Reader.data should be sufficient.

richardjgowers added a commit that referenced this issue Jul 19, 2015
fixed, skip_timestep, periodic, skip and delta no longer required for
Readers (Issue #350)

All writers now refer to time between steps as "dt" (was previously
delta in XTC, TRR and NCDF Writers) (Issue #206)

Timesteps now have a default dt of 1.0 ps (and issue warning when using
this)

Timestep attribute time now tries to return data['time'] first, then dt
* frame, using the default value of 1.0 ps if necessary. (Issue #308)
@dotsdl dotsdl reopened this Jul 21, 2015
@dotsdl
Copy link
Member

dotsdl commented Jul 21, 2015

The list of attributes for Readers given here needs to be updated, right?

@dotsdl dotsdl closed this as completed Jul 21, 2015
@dotsdl
Copy link
Member

dotsdl commented Jul 21, 2015

Scratch that...had a look at the actual doc source. Hadn't occurred to me that we might not have regenerated the development docs yet.

orbeckst added a commit that referenced this issue Jul 22, 2015
@orbeckst
Copy link
Member

Ah... ok, I just regenerated the docs.

@richardjgowers
Copy link
Member Author

Oops, forgot to regen docs. So if I've read #183 right, the reason we can't have online docs is the C extensions. Would it be possible to edit setup to build a (non working) version of MDA without C extensions?
ie

python setup.py install --for-docs

Then as long as we don't have docs inside C modules (or Cython I imagine), we can create enough for the docs to be built?

@orbeckst
Copy link
Member

The way yo go about this is to make travis-ci build the docs (and possibly even deploy). Rob McGibbon wrote about this somewhere and I think it's a great idea.

Oliver

On 22 Jul, 2015, at 02:14, Richard Gowers wrote:

Oops, forgot to regen docs. So if I've read #183 right, the reason we can't have online docs is the C extensions. Would it be possible to edit setup to build a (non working) version of MDA without C extensions?

ie

python setup.py install --for-docs
Then as long as we don't have docs inside C modules (or Cython I imagine), we can create enough for the docs to be built?


Reply to this email directly or view it on GitHub.

Oliver Beckstein * orbeckst@gmx.net
skype: orbeckst * orbeckst@gmail.com

@orbeckst
Copy link
Member

@richardjgowers : Update to my last comment: @rmcgibbo described how to deploy docs and binaries with travis-ci in mdtraj. I thought that was very neat.
I don't know if we can make this work with the current workflow where devdocs are part of the git repo but it's worth thinking about.

@richardjgowers
Copy link
Member Author

Yeah, I was looking through the travis.yml on mdtraj, lots of cool hooks in their after_success.

I think it'd hinge on whether we could push to gh-pages as travis.. and my initial guess would be no because of keys/permissions etc, but it looks like there's such a thing as secure travis variables, which would then seem to make it possible.

@rmcgibbo
Copy link
Contributor

It looks like other people have written about pushing to gh-pages from travis (e.g. here and here).

The one tricky issue that we had with the docs that's worth noting is that the implementation of the ability to move between different versions of the docs (e.g. the dropdown "Versions" menu in the lower left corner of mdtraj.org) is tricky. Since the site is static, you need the fixed old versions of the docs to "become aware" of new versions to properly populate the dropdown menu, so it needs to all be done dynamically in js. And if you realize that there's a bug in the js in a ~6 month old docs build of a prior release, it's kind of hard to fix.

@richardjgowers
Copy link
Member Author

Cool thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants