schrodinger / coordgenlibs

Schrodinger-developed 2D Coordinate Generation
BSD 3-Clause "New" or "Revised" License
42 stars 28 forks source link

avoid setting bonds info when reading in atom stereochemistry #32

Closed ZontaNicola closed 5 years ago

ZontaNicola commented 5 years ago

writeStereoChemistry() was setting bond direction flags (inaccurately cause at this stage atoms usually have no coordinates) that are not cleaned up and can end up messing the stereochemistry of the final molecule. (bond directions are set at a later stage)

ZontaNicola commented 5 years ago

Do you have an understanding of why the test failed? Do you want Ricardo or me to look at it?

I have no idea, it looks like it's related to the linking of maeparser, it's strange that it would be triggered by this. If you guys could have a look, that would be awesome

ricrogz commented 5 years ago

Do you have an understanding of why the test failed? Do you want Ricardo or me to look at it?

I have no idea, it looks like it's related to the linking of maeparser, it's strange that it would be triggered by this. If you guys could have a look, that would be awesome

I checked it this morning: apparently Travis has changed the default Ubuntu version to Xenial, so that the default boost library is now built with gcc/g++ v5, and doesn't link well with gcc/g++ v4.8, which we enforce in the builds.

I already opened pull requests in maeparser and coordgenlibs to update and build in the three available Ubuntu LTS releases (each one with their fitting compilers and libs)

d-b-w commented 5 years ago

I've merged @ricrogz 's commit to fix the build. @ZontaNicola - would you mind bringing this branch up to date with master so that the tests run?

ZontaNicola commented 5 years ago

Sure, looks like the tests are passing now, thanks guys!

On 6 Aug 2019, at 13:06, Dan N notifications@github.com wrote:

I've merged @ricrogz https://github.com/ricrogz 's commit to fix the build. @ZontaNicola https://github.com/ZontaNicola - would you mind bringing this branch up to date with master so that the tests run?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/schrodinger/coordgenlibs/pull/32?email_source=notifications&email_token=ACMX3KGX6MMDTORZ5IQ74ULQDHDR3A5CNFSM4IIAYPJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3WEYHI#issuecomment-518802461, or mute the thread https://github.com/notifications/unsubscribe-auth/ACMX3KCMJM6GVCSQ53ENKULQDHDR3ANCNFSM4IIAYPJA.

d-b-w commented 5 years ago

Nic- Would you please add a test for this?

d-b-w commented 5 years ago

Ricardo's added a simple demo test that reads an mae file. Is that a sufficient base to add a test for this bug?