search

UC-Davis-molecular-computing / scadnano-python-package

Python scripting library for generating designs readable by scadnano.
https://scadnano.org
MIT License
14 stars 7 forks source link

closes #200: make scadnano pitch angle agree with oxDNA #201

Closed DanielHader closed 2 years ago

DanielHader commented 2 years ago

closes #200: make scadnano pitch angle agree with oxDNA

DanielHader commented 2 years ago

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 https://github.com/UC-Davis-molecular-computing/scadnano-python-package/pull/201#discussion_r745895582 :

  • 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 https://github.com/UC-Davis-molecular-computing/scadnano/issues/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 https://github.com/UC-Davis-molecular-computing/scadnano-python-package/pull/201#pullrequestreview-801663261, 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.