Skip to content
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

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

tcosmo
Copy link
Collaborator

@tcosmo tcosmo commented Feb 1, 2022

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 :

Screenshot 2022-02-01 at 17 52 26

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):

@dave-doty dave-doty merged commit 123a549 into dev Feb 1, 2022
@dave-doty dave-doty deleted the bug_cadnano2_format_same_helix_xsover branch February 1, 2022 19:12
Copy link
Collaborator

@UnHumbleBen UnHumbleBen left a 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"
Copy link
Collaborator

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"
Copy link
Collaborator

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"

Copy link
Member

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,
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants