Skip to content

CRDGEN-270#87

Merged
d-b-w merged 3 commits intoschrodinger:masterfrom
ZontaNicola:master
Mar 30, 2021
Merged

CRDGEN-270#87
d-b-w merged 3 commits intoschrodinger:masterfrom
ZontaNicola:master

Conversation

@ZontaNicola
Copy link
Copy Markdown
Collaborator

I added a template for C1C2CC1C2 aka bicycle (1.1.1) pentane. I had to change the ring system semplification logic slightly so that the template is picked up.

@ZontaNicola ZontaNicola requested a review from d-b-w March 26, 2021 16:48
@d-b-w d-b-w requested a review from rachelnwalker March 26, 2021 17:09
Copy link
Copy Markdown
Collaborator

@d-b-w d-b-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for this.

Comment thread templates.mae Outdated
Comment thread CoordgenFragmentBuilder.cpp
@d-b-w
Copy link
Copy Markdown
Collaborator

d-b-w commented Mar 29, 2021

@ZontaNicola -
I merged the SMILES test generator PR if you want to use that to write a test for this.

@ZontaNicola ZontaNicola requested a review from d-b-w March 29, 2021 15:49
@ZontaNicola
Copy link
Copy Markdown
Collaborator Author

@d-b-w I can see the commit with the test, does it not show up for you?

Comment thread test/test_coordgen.cpp
BOOST_TEST(bridgeAtom1->neighbors.size() == 2);
BOOST_TEST(bridgeAtom2->neighbors.size() == 2);
BOOST_TEST(bridgeAtom3->neighbors.size() == 2);
auto distance1 = (bridgeAtom1->getCoordinates() - bridgeAtom2->getCoordinates()).length();
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 this be a good test case for areBondsNearIdeal()?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, these atoms are not bonded together and the distance is different from a bond lengths. This is testing that they are not too close to each other and drawn on top of each other

Copy link
Copy Markdown
Collaborator

@d-b-w d-b-w 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 adding the test, I totally missed it this morning when I was looking.

Comment thread templates.mae
}
}

f_m_ct {
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.

Does the order of the templates in this file matter?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they get matched in order, so theoretically I guess more frequent templates should go first, but in practice I don't think it would make a difference

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.

thanks, @ZontaNicola . I guess what I was worried about is if there are races between templates, what happens?

@d-b-w d-b-w merged commit f67ad5d into schrodinger:master Mar 30, 2021
d-b-w added a commit that referenced this pull request Apr 1, 2021
For use in Schrödinger release 2021-1.

Set version in CMakelists to 2.0.1. This release includes a fix to rendering of bicyclic pentane (#87).
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