Repository navigation
maskedarray fixes; bugfix in minus ensemble - #803
Conversation
Using masked arrays, even when we say not to? This forces things to be normal numpy arrays.
Doesn't conflict with strategies
Use the internal _new_ensemble's str. Otherwise default is just "Ensemble", and since equality is based on ensemble name, that's bad (may need to change the default ensemble name anyway)
|
This looks reasonable, although I fear that it might add some decrease in performance. |
|
Would it be possible to remove masked arrays altogether? |
|
On the performance hit, I don't think it'll be too bad (because this is just for things like OpenMM, where the dynamics should be the primary cost -- However, the commands that were supposed to do that don't seem to work. Since this is the second time that I've had to fix this after netcdf broke it for us (without even a major version change), I went for an approach that should fix the problem, regardless of what they do in the future. (You might guess that I'm a little frustrated with the netcdf team.) I'm still trying to fix the last bit of the minus ensemble problem; I was still getting the error due to what looks like a problem with deserializing the ensemble. I'm running tests now to see if I've fixed that. |
dwhswenson
changed the title
|
This now seems to work (at least, I can run the AD-MSTIS example). Barring objections, I will merge this on Monday (and release 0.9.6). @jhprinz, I fully agree that there might be a better solution to this, but I think getting some sort of fix out is urgent. If you find a better fix to this, please make a PR, because I'd prefer to just have netcdf return normal numpy arrays. Unfortunately, I couldn't find an easy way to make that happen. |
2 participants
This solves two problems that were brought to our attention by @uscgrad (thanks Linkel!)
For some reason, in Python 3, netcdf was ignoring our request to not use masked arrays. (See also Fix problems in to_mdtraj and notebook tests #776). This was leading to errors when treating the box vectors as numpy objects due to the issues between numpy
maskedarraysandsimtk.unit.Quantity. The solution is to forcesnapshot.box_vector,snapshot.velocities, andsnapshot.coordinates(as used by the OpenMM engine) to returnsimtk.unit.Quantityobjects wrapping a normalnumpy.arrayinstead of amaskedarray. See the new functionunmask_quantityfor details. Note that I couldn't figure out how to reproduce the error in a simple test, so there are no unit tests for this.There seems to have been a bug in the way networks create minus interfaces. The minus interface is supposed to stop upon entry to any other state, however, this isn't what was implemented. As a practical matter, this wouldn't have mattered for any realistic system, because the maximum path length should be much less than the lifetime of a state (but alanine dipeptide just isn't a rare enough event). However, for events that aren't actually that rare (such as in alanine dipeptide), the assumptions required by the
candidatetrajectories introduced in Fast ensemble check #669 aren't correct. The fix is to change the minus ensemble such that the assumptions are correct. This PR does that, and includes unit tests to ensure it doesn't become a problem again.I'm currently re-running the AD MSTIS example (Py3, 10k MC steps). It worked fine when I ran it with 1k MC steps (though that might have been Py2). Code here is ready for review; only the notebooks will be changing once I clean them up.
Also includes some PEP8 cleanup.