closes #200: make scadnano pitch angle agree with oxDNA#201
Merged
Conversation
dave-doty
requested changes
Nov 9, 2021
Member
dave-doty
left a comment
There was a problem hiding this comment.
Doesn't seem to work as we want. See other comment below.
It might be a good idea to make up some small unit tests to express intended behavior. I'm not quite sure what good unit tests would look like, since testing exact float-point positions of bases after the rotation seems brittle and difficult to figure out.
scadnano/scadnano.py
Outdated
|
|
||
| # then the roll rotation | ||
| yaw_axis = yaw_axis.rotate(-design.roll_of_helix(helix), roll_axis) | ||
| pitch_axis = pitch_axis.rotate(-design.roll_of_helix(helix), roll_axis) |
Member
There was a problem hiding this comment.
This code seems suspicious to me. The variable pitch_axis is not used after this point, so if assigning to it is supposed to have some effect, it won't.
Also, it isn't what I was expecting. An example similar to that shown in UC-Davis-molecular-computing/scadnano#667, which looks like this in scadnano (second helix group has pitch=30):
now displays in oxDNA like this:
It should look like this (but with the blue axis pointing right and the green axis pointing down):
Collaborator
Author
|
Oops, sorry about that! I accidentally switched the x and z axes which is
why it looked right, but in the wrong orientation. I just committed a fixed
version.
I also removed the last write to the pitch_axis variable which I left in
when I was making changes to the code originally. I thought I might use it
later so I updated it, but I never ended up using it.
Yeah, I'm not really sure what good unit tests would be for this, I could
check for floating point equality upto some small epsilon which should be
good enough to check against things like rotations being backwards.
Thanks,
Daniel
…On Tue, Nov 9, 2021 at 12:32 PM David Doty ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Doesn't seem to work as we want. See other comment below.
It might be a good idea to make up some small unit tests to express
intended behavior. I'm not quite sure what good unit tests would look like,
since testing exact float-point positions of bases after the rotation seems
brittle and difficult to figure out.
------------------------------
In scadnano/scadnano.py
<#201 (comment)>
:
>
+ # we apply rotations in the order yaw, pitch, and then roll
+ # the _OxdnaVector.rotate function applies ccw rotation so angle needs to be negated
+
+ # first the yaw rotation
+ pitch_axis = pitch_axis.rotate(-design.yaw_of_helix(helix), yaw_axis)
+ roll_axis = roll_axis.rotate(-design.yaw_of_helix(helix), yaw_axis)
+
+ # then the pitch rotation
+ yaw_axis = yaw_axis.rotate(-design.pitch_of_helix(helix), pitch_axis)
+ roll_axis = roll_axis.rotate(-design.pitch_of_helix(helix), pitch_axis)
+
+ # then the roll rotation
+ yaw_axis = yaw_axis.rotate(-design.roll_of_helix(helix), roll_axis)
+ pitch_axis = pitch_axis.rotate(-design.roll_of_helix(helix), roll_axis)
This code seems suspicious to me. The variable pitch_axis is not used
after this point, so if assigning to it is supposed to have some effect, it
won't.
Also, it isn't what I was expecting. The example shown in
UC-Davis-molecular-computing/scadnano#667
<UC-Davis-molecular-computing/scadnano#667>,
which looks like this in scadnano:
[image: image]
<https://user-images.githubusercontent.com/19274365/140983235-3b920747-32d5-4467-a077-c57f3d099bf6.png>
now displays in oxDNA like this:
[image: image]
<https://user-images.githubusercontent.com/19274365/140982906-f2226d47-d99a-4d4e-9df5-5e42cb843ca0.png>
It should look like this (but with the blue axis pointing right and the
green axis pointing down):
[image: image]
<https://user-images.githubusercontent.com/19274365/140983139-776c66a2-e42c-48cc-8a09-2f0ab47ec234.png>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#201 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABA6CNVTBKOAEIIKMXADZGLULFSMTANCNFSM5HV3CG4Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
dave-doty
approved these changes
Nov 12, 2021
Member
dave-doty
left a comment
There was a problem hiding this comment.
I implemented it in Dart and played with it a bit, and it seems to give expected results. It would be nice to have some unit tests, but like I said I'm not sure how to make them robust.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #200: make scadnano pitch angle agree with oxDNA