Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2515 +/- ##
===========================================
+ Coverage 90.25% 91.07% +0.81%
===========================================
Files 170 170
Lines 22874 23070 +196
Branches 2989 3028 +39
===========================================
+ Hits 20646 21011 +365
+ Misses 1625 1467 -158
+ Partials 603 592 -11
Continue to review full report at Codecov.
|
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
lgtm, just a couple of minor docstring suggestions.
package/CHANGELOG
Outdated
| * Removed core.flags. Default behaviour (eg pbc, units) now defaults to | ||
| the default setting of the flag before removal. If you were using flags | ||
| you will now have to supply the flag setting as keyword arguments to | ||
| function calls. |
There was a problem hiding this comment.
Would it be worth adding the issue number here?
| @@ -148,10 +147,8 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=None, | |||
| """ | |||
There was a problem hiding this comment.
Whilst convert_units wasn't part of the __init__ docstring originally, would it be worth adding it?
There was a problem hiding this comment.
This is on the Writer, so this being true would convert inputs to the native units ... for xyz? I'm not sure if XYZ has a native unit? So I'm not going to break anything here, but I'm not sure I want to document the fact that this option exists?
There was a problem hiding this comment.
XYZ units are generally in angstroms and that's how most visualization packages interpret them (VMD for example).
There was a problem hiding this comment.
So MDA does seem to assume ps/Angstrom on read, and the default writer also assume those units on the conversion. I guess the only case I could think of where this might be useful, is if for some reason you created a Universe with non-base units, and then wanted to use the writer to have a non-ps/Angstrom write. With the removal of flags, I'm not sure how likely that would be to happen though...
| @@ -2090,13 +2089,10 @@ class ReaderBase(ProtoReader): | |||
|
|
|||
There was a problem hiding this comment.
Since these are base readers, would it be worth documenting the slight behaviour change as a versionchanged?
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
lgtm — one minor typesetting comment...
You're a hero, you shall be known henceforth as the Slayer of Ill-Conceived and Misguided Features.
| be calculated without moving any :class:`Atoms<Atom>` to keep the | ||
| compounds intact. Instead, the resulting position vectors will be | ||
| moved to the primary unit cell after calculation. | ||
| moved to the primary unit cell after calculation. Default False. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Well, I totally forgot that we also need to kill documentation such as
to highlight periodic (and other) kwargs
|
@orbeckst ok docs now all done, grepping doesn't show any more flag stuff in the repo |
|
Thanks go ahead and do what needs to be done. (I’m traveling and on mobile...) |
fiona-naughton
added
maintainability
Component-Core
deprecation
5 participants
Fixes #782
Changes made in this Pull Request:
PR Checklist