Sgroup #149

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants
@johnmay
Member

johnmay commented Sep 4, 2015

Adds preliminary support for Molfile Sgroups. Currently only round tripping and rendering. Rendering is limited to polymer brackets but I plan on adding the other rendering parts later this week.

Lots more todo but gives us a start. I've wanted to add this for some time and meeting Venkat at the ACS he said they couldn't use CDK in ChEBI because it didn't display Sgroups.

Molfile SRU example

johnmay added some commits Sep 2, 2015

For now Sgroups are stored on a property of the AtomContainer. A more…
… specific API (need interfaces) may be added in future.
Abbreviations have some additional attributes (EXP=expand,SAP=attach …
…point,SBV=bond vector) we don't yet handle. We can store whether an Sgroup is expanded easily but the others will need additional classes. Since these only seem to be used by more complex (e.g. peptide) contractions they are ommitted for now.
@egonw

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

I guess you considered the option of extending IAtomContainer? With "public IAtomContainer getParents()" ?

I guess you considered the option of extending IAtomContainer? With "public IAtomContainer getParents()" ?

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

I good argument to not do this, is that you need to select an implementation to extend too... which would severely complicate things...

Member

egonw replied Sep 6, 2015

I good argument to not do this, is that you need to select an implementation to extend too... which would severely complicate things...

This comment has been minimized.

Show comment
Hide comment
@johnmay

johnmay Sep 6, 2015

Member

The bonds here aren't really the bonds to the substructure though.. so that abstraction wouldn't make sense.

Member

johnmay replied Sep 6, 2015

The bonds here aren't really the bonds to the substructure though.. so that abstraction wouldn't make sense.

@egonw

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

Personally, I prefer (int value, int length)...

Personally, I prefer (int value, int length)...

This comment has been minimized.

Show comment
Hide comment
@johnmay

johnmay Sep 6, 2015

Member

Not mine - sorry formatting got caught up. Also noticed this code could be optimised since all MDL ints are 2 or 3 digits and floats are 10 we can reuse the formatting objects.

Member

johnmay replied Sep 6, 2015

Not mine - sorry formatting got caught up. Also noticed this code could be optimised since all MDL ints are 2 or 3 digits and floats are 10 we can reuse the formatting objects.

@egonw

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

Just wondering... do you know if there exist PMD tests that encourage using this syntax over assertTrue(output.contains("M SAL ...")) ?

Just wondering... do you know if there exist PMD tests that encourage using this syntax over assertTrue(output.contains("M SAL ...")) ?

This comment has been minimized.

Show comment
Hide comment
@johnmay

johnmay Sep 6, 2015

Member

Don't think so - it's nice though because it shows what the string was if it didn't find it :-).

Member

johnmay replied Sep 6, 2015

Don't think so - it's nice though because it shows what the string was if it didn't find it :-).

@egonw

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

Another thought... we're Java7 now, right? PMD used to have "migration" tests for moving to Java7... I would need to check if they have tests for migration to Java7, to highlight things that can be done more efficiently, like you do here...

Another thought... we're Java7 now, right? PMD used to have "migration" tests for moving to Java7... I would need to check if they have tests for migration to Java7, to highlight things that can be done more efficiently, like you do here...

@egonw

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

Never mind my comments... they are not requests. Code looks good. I will now try to apply it locally.

Member

egonw commented Sep 6, 2015

Never mind my comments... they are not requests. Code looks good. I will now try to apply it locally.

@egonw

This comment has been minimized.

Show comment
Hide comment
@egonw

egonw Sep 6, 2015

Member

Applied (rebased) and pushed.

Member

egonw commented Sep 6, 2015

Applied (rebased) and pushed.

@egonw egonw closed this Sep 6, 2015

@johnmay johnmay deleted the sgroup branch Sep 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment