Skip to content

Cadnano2 import of crossover to same helix#214

Merged
dave-doty merged 1 commit intodevfrom
bug_cadnano2_format_same_helix_xsover
Feb 1, 2022
Merged

Cadnano2 import of crossover to same helix#214
dave-doty merged 1 commit intodevfrom
bug_cadnano2_format_same_helix_xsover

Conversation

@tcosmo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.)

#
# 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
Copy Markdown
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