-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cadnano2 import of crossover to same helix #214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Just reading it over while working on porting this code to Dart and have a few changes to request.
Edit: I didn't realize Dave already went ahead and ported it over to Dart 😅
|
||
def test_paranemic_crossover(self) -> None: | ||
file_name = "test_paranemic_crossover" | ||
file_name = "test_crossover_to_same_helix" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo? Seems like file name should be "test_paranemic_crossover" instead of "test_crossover_to_same_helix"
filename=f'{file_name}.{sc.default_scadnano_file_extension}') | ||
|
||
def test_same_helix_crossover(self) -> None: | ||
file_name = "test_paranemic_crossover" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo? Seems like file name should be "test_same_helix_crossover" instead of "test_paranemic_crossover"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call it test_same_helix_crossover
. A paranemic crossover is one that connects domains going the same direction on both helices, which is technically true here, but this is more a "fake" crossover that doesn't really represent the DNA geometry. (Unlike paranemic crossover, which is an actual DNA motif.)
@@ -604,8 +604,8 @@ def test_32_helix_rectangle(self) -> None: | |||
# To help with debugging, uncomment these lines to write out the | |||
# scadnano file | |||
# | |||
# design.write_scadnano_file(directory=self.output_path, | |||
# filename=f'test_32_helix_rectangle.{sc.default_scadnano_file_extension}') | |||
design.write_scadnano_file(directory=self.output_path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to leave these write_scadnano_file
lines commented? They are only there for debugging purposes. We don't want unit tests to write files that we don't need.
Description
Crossovers to same helix as in attached filed were badly handled.
crossover-to-same-helix.zip
The fix consisted in adding one more case to the condition that decides whether or not to add a crossover :
I have added hopefully clearer comments.
Related Issue
#209
Motivation and Context
Fixes #209
How Has This Been Tested?
tests.scadnano_tests.TestImportCadnanoV2.test_crossover_to_same_helix.sc
Screenshots (if appropriate):