Feature bonds #121
Feature bonds #121
Conversation
…gement of options.
the dynamics code. Added tests and source documentation.
…86 using CPU [ci skip]
of the 3 angles. The angles are converted from degrees to radians when held internally. But they should exactly convert back to the degrees value via `ang.to(degrees)` as I think division/multiplication by the same number is reversible. [ci skip]
[ci skip]
part of the unit test that checks that velocity rotation is working.
[ci skip]
indexing functions so that you can pass in an existing view or views as
the key. This lets you look up views in a container by other views, e.g.
`mols.bond(mols.atoms()[0], mols.atoms()[1])` would return the bond
between the first two atoms in the container `mols`.
Also added an `error_on_missing` flag to the `atoms`, `residues`, `bonds` etc
functions, so that you get a `KeyError` exception if there is no match,
and `error_on_missing` is `True`.
For example, `mols.atoms("element C", error_on_missing=True)` would raise
an exception if there are no carbon atoms in this container.
This is default `False` to keep existing behaviour, but we would recommend
setting this to `True` and would like to change the default in the future.
There was a problem hiding this comment.
Nice, this is a really handy piece of functionality. I've tested locally and everything is working as expected. The changes from the other PRs will need manual adjustments behind the scenes in BioSimSpace once this is merged. I'll also need to find some example input so that they can be tested properly.
My only comment is whether the cursor rotation should have any auto-detection for perturbable molecules, or whether this should be left to the user to do, e.g. via the property map. This is what I currently do in my workaround, i.e. rotate 4 times, mapping coordinates to coordinates0, velocity0, coordinates1, and velocity1. Perhaps it could check for the is_perturbable property then search for properties matching map.get("coordinates", "coordinates") + "0" (and lambda 1), etc.
|
Thanks - that's a good question. I think the code shouldn't try to be that clever. If the properties are linked, then the rotation would only apply to the coordinates/velocities of the linked state (i.e. the reference or perturbed state). I can imagine situation where the user may want to rotate or translate the end states differently. I can also imagine situations where there's more than one end state, or we have completely different types of merged or complex molecules we want to support. Given this, I think it is better to be explicit rather than implicit, and that it should be left to the user to do two rotations if they want both end states changed. |
Changes proposed in this pull request:
Extended the
.atom(s),.residue(s),.bond(s)and all other indexing functions so that you can pass in an existing view or views as the key. This lets you look up views in a container by other views, e.g.mols.bond(mols.atoms()[0], mols.atoms()[1])would return the bond between the first two atoms in the containermols.Also added an
error_on_missingflag to theatoms,residues,bondsetc functions, so that you get aKeyErrorexception if there is no match, anderror_on_missingisTrue.For example,
mols.atoms("element C", error_on_missing=True)would raise an exception if there are no carbon atoms in this container.This is default
Falseto keep existing behaviour, but we would recommend setting this toTrueand would like to change the default in the future.develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]Suggested reviewers:
@lohedges
Any additional context of information?
I've not added documentation as this puts in functionality that makes the code behave more intuitively. I added this because I found that
mols.bond(mols.atoms()[0], mols.atoms()[1])didn't work, even though, intuitively, it should.This builds on the other PRs. For ease of review, here's the one commit that is new with this PR: 599b2eb