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

first release #48

Closed
orbeckst opened this issue Mar 31, 2016 · 12 comments
Closed

first release #48

orbeckst opened this issue Mar 31, 2016 · 12 comments
Milestone

Comments

@orbeckst
Copy link
Contributor

What is holding up a release?

@orbeckst orbeckst added this to the 0.6.0 milestone Mar 31, 2016
@dotsdl
Copy link
Member

dotsdl commented Mar 31, 2016

It's shipping today; working on docs and some final decisions for how limbs should work for a package like mdsynthesis to avoid namespace clashes with upstream limbs. Ran out of time yesterday before finishing this work up.

@dotsdl
Copy link
Member

dotsdl commented Apr 1, 2016

Docs are done as far as I can tell; please give a quick read and see if anything feels missing or off. I'm too involved to have fresh eyes on them: http://mdsynthesis.readthedocs.org/en/develop/

@richardjgowers
Copy link
Contributor

Not strictly related, but the links in the readme on the repo are a little borked. datreant_ has an underscore and there's a :ref: somewhere

@dotsdl
Copy link
Member

dotsdl commented Apr 1, 2016

@richardjgowers thanks for catching that. The README isn't the point of entry for users, but I did lift some of the rewritten intro text from the docs and put it there. Forgot to replace the sphinx-parsed bits with components that would work for raw RST.

@orbeckst
Copy link
Contributor Author

orbeckst commented Apr 1, 2016

docs:

  1. I would make the first mentioning of Sim an actual link to the class definition
  2. dependency: MDAnalysis >= 0.15? That's not even out yet...
  3. Also make Sim a link in the Leveraging Sims section. Can you use intersphinx links to make MDAnalysis.Universe a link? (perhaps use ~MDAnalysis.core.AtomGroup.Universe?)
  4. Treant and Sim are not always marked up.
  5. Too much talking about Treant objects --- focus on the MDSynthesis user who comes to the package the first time and does not want to (or need to) dive into datreant. I think the docs will need some rewriting to get rid of the developer point of view but don't do this now... for the initial release it will be fine and it's not too bad. In fact, if you just say all the things about "Sims are Treants" after having explained what you do with Sims then it would be fine. (And don't explain Sims by contrasting with Treants... not helpful for people who don't know Treants.)
  6. s.udef vs s.universe is very confusing: If I didn't know any better I'd do s.universe.trajectory = "path/to/xtc"... and it would even work with disastrous consequences. Perhaps making udef.trajectory a property is hiding to much (e.g. assigning reloads the universe?... that makes sense but seems to be a big side-effect; an explicit method call like load_trajectory() might be clearer)
  7. Come to think, I guess I just dislike s.udef because neither the name nor the docs tell me what it is good for.
  8. s.atomselections is really weird, syntactically and semantically: I assign a strong or a list and then I get back an AtomGroup?? I think you're abusing properties here but that might just be me. (At least you can assign an AtomGroup, so there's some balance.) I am not a fan of s.define(key) --- it seems decoupled from atomselections. But I don't have a good solution: making s.atomselections[key] a proper object allows one to do s.atomselections[key].definition = "selection string" (which is nice) but then s.atomselections[key].atoms, which seems clunky: just s.atomselections[key] would be nicer. Or make s.atoms[key] available for each s.atomselections[key].atoms...
  9. mdsynthesis.limbs.AtomSelections: The name define() has the wrong semantics: You are not issuing a command to define but you are getting a definition back. It should be get_definition or perhaps just a dict-like lookup s.definitions[key] (and you're even raising a KeyError... hint, hint!)... and see above. When I encounter semantic shear in a UI (i.e. the function is different or the opposite from what the words mean in normal English) I cringe.

@orbeckst
Copy link
Contributor Author

orbeckst commented Apr 1, 2016

@dotsdl , it's very useful to have the UI laid out in the docs, so your docs are a success. But seeing it in this way, in context, in summary, made me realize that there are some UI choices that still feel rough or kludgey. Apologies if any or all of this had already been debated and settled previously.

@orbeckst
Copy link
Contributor Author

orbeckst commented Apr 1, 2016

docs:

  • bottom: © Copyright 2014 ... shouldn't this be 2016?
  • have a link to the GitHub repo on the first page in the text: you have datreant and MDAnalysis... but not MDS itself.

dotsdl added a commit that referenced this issue Apr 1, 2016
dotsdl added a commit that referenced this issue Apr 1, 2016
@dotsdl
Copy link
Member

dotsdl commented Apr 1, 2016

@orbeckst this is all extremely helpful. Thanks!

  1. Fixed.
  2. Oops...I clearly counted up one too far. I should know this. Set to 0.14
  3. Yeah, we use intersphinx already; made a proper link to the module where Universe lives.
  4. Should they be? I tend to find it distracting if they're always plaintexted or always links. If there's consensus to do this (e.g. instead of Treant, we always say Treant), then I'll do it.
  5. This is a hard problem, and I don't have a good solution for it. Much of the non MD-specific functionality, which is to say 90% of what a Sim can do, is defined in datreant, and doing any kind of help(Sim.<methodname>) will do lots of talking about Treants, Trees, Views, etc. I think a downside to how datreant and MDSynthesis are designed is that you can't completely hide the inner library within the outer one without a lot of work that defeats the point (lots of duplication). All ears for how to sustainably go about this, though.
  6. I agree its confusing, but a way of partitioning these things in the brain are that udef is the Sim's universe definition: the description of what it needs to generate its universe. The Sim's universe is the result of that definition, and it should always reflect that definition. Perhaps there are better ways of doing this, but part of the reason for this is that we want to limit how much Sim imlements into its own namespace to avoid clashes with upstream datreant. A Sim gives only three components in its namespace beyond what a Treant has: universe, udef, and atomselections. All three of these are unlikely to every clash with upstream limbs.
  7. See above; if I make this way of viewing udef explicit in the docs, would that be a solution for this? Or should we completely rethink this, perhaps by monkey-patching the Universe such that Sim.universe.topology = 'path/to/top.psf' just works instead (or as well)?
  8. I agree that this is asymmetric, and I don't like that generally. Alternatively one can always do s.atomselections.add('lid', 'resid 122:159') to achieve the same effect as s.atomselections['lid'] = 'resid 122:159'. Perhaps we could add an atoms limb as you suggest that gets back AtomGroups from stored atom selections, leaving atomselections to work only with the definitions? It'd be a bit different from previous limbs in that it interacts with the data stored by another, but it would work.
  9. I agree; I've always hated the semantics of this name, but couldn't think of something that doesn't feel clunky (although they're traditional, I think get_* are a bit ugly). Depending on (8) above, this may be moot. Perhaps we should go that route, separating definition limb (atomselections) from object limb (atoms), in the same way as we do for the Universe (udef and universe)? That would bring some consistency on one dimension.

@richardjgowers
Copy link
Contributor

@dotsdl with 6 & 7, can we not just make udef hidden to the user and just make everything work through assigning a mda.Universe?

@dotsdl
Copy link
Member

dotsdl commented Apr 2, 2016

@richardjgowers I actually really like that idea, and I think we can go ahead with it and see how far we get. If it turns out we need a place for tweaking how the Universe behaves that can't be done from or with the Universe itself, then we can expose udef (remove the underscore, really).

This can go a long way toward including keyword argument persistence (see #25), since the solution arrived at for how the Universe stores these (MDAnalysis/mdanalysis#292) can be made to only have strings, bools, floats, and ints (some kwargs can be objects, so must be a mapping for those (I think we already have this for Readers)) so it is immediately extractable and storable by the Sim inside its JSON state file.

Sound good?

dotsdl added a commit that referenced this issue Apr 4, 2016
We will encourage setting a Sim universe directly with an existing
Universe, with the hope that we can eventually hide UniverseDefinition.
At the moment, though, we leave it exposed for any manual interventions
that can't be handled from the Universe itself.

Also, AtomSelection getitem no longer returns an AtomGroup, but instead
gives back the definition of the selection (that which was stored). It
is possible to store only strings or numpy arrays of atom indices,
or tuples/lists of these in any combination. One cannot directly store
AtomGroups, but can store atom indices with `AG.indices`.

Getting back an AtomGroup from the stored definition requires using the
`create` method. This makes it clear that a new AtomGroup is being
generated, and that there is a cost to this.
@dotsdl
Copy link
Member

dotsdl commented Apr 5, 2016

Alright, pushing to PyPI. Thanks all!

@dotsdl dotsdl closed this as completed Apr 5, 2016
@orbeckst
Copy link
Contributor Author

orbeckst commented Apr 5, 2016

Hooray!

(And you even managed to close all issues in the tracker...)

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

3 participants