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

Modifying a molecule should not automatically clear SubstanceGroups #3168

Closed
greglandrum opened this issue May 14, 2020 · 3 comments · Fixed by #3170
Closed

Modifying a molecule should not automatically clear SubstanceGroups #3168

greglandrum opened this issue May 14, 2020 · 3 comments · Fixed by #3170
Assignees
Milestone

Comments

@greglandrum
Copy link
Member

The current behavior - clearing all SubstanceGroups whenever a molecule is modified - was a sensible initial implementation strategy given that we were still gaining experience with the use cases for SubstanceGroups. Taking a "better safe than sorry" approach made sense.

Unfortunately, this decision has some fairly unfortunate side effects: if a molecule includes any Hs, all SubstanceGroups will be removed when the Hs are removed, which is the default behavior. This happens regardless of whether or not the Hs are involved in the SubstanceGroups.

I think this has a large enough impact on the usefulness of SubstanceGroups that it should be fixed.

We can argue aboutdiscuss whether or not this is actually a bug since the current behavior is intentional. I think that from the user perspective it feels like a bug when all SubstanceGroups vanish just because a molecule contained one or more H atoms that weren't involved in the SubstanceGroups.

The next argumentdiscussion is whether or not this should be fixed in a patch release. I would like to do so because I need to be able to use this fix in a project I'm working on. That's not a good argument if it introduces potential problems for the broader community. However, I would guess that the SubstanceGroup functionality is currently not heavily used by the community. It's something of an advanced user thing and is hardly documented. Still, if this makes the other maintainers uneasy, we can shift this out to the 2020.09 release and I will set up a fork for my other project.

@greglandrum
Copy link
Member Author

@ricrogz, @d-b-w : you guys may have some thoughts/opinions on this one.

@greglandrum
Copy link
Member Author

This is related to #2716

greglandrum added a commit to greglandrum/rdkit that referenced this issue May 14, 2020
@ricrogz
Copy link
Contributor

ricrogz commented May 14, 2020

I think it makes sense to do this sooner than later.

As you say, I don't think stripping the Substance Groups on modification of the molecule is a bug, since we decided it to be that way, although them getting stripped from molecules with H atoms is, since that is an unplanned side effect of that decision.

I think the best way to fix the H atoms problem is to implement the code that would allow modification of the molecule without losing the Substance Groups, so I think it makes sense to include it in a patch release.

I don't think that would impact the user community in a bad way, because, as you said, Substance Groups are mostly undocumented, and not widely used. And, even for the few people that use them, this change would be beneficial, since I guess the bug with the H atoms is annoying for them.

greglandrum added a commit that referenced this issue May 19, 2020
* Progress on #3168

* Fixes #3167

* Fixes #3169

* deal with CBONDS too

* test PATOMS

* Fixes #3175

* a bit of code simplification and test updates

still needs more testing

* more testing

* handle s-group hierarchy
also a couple of other changes in response to the review

* add forgotten test file

* changes in response to review
greglandrum added a commit that referenced this issue Jun 9, 2020
* Progress on #3168

* Fixes #3167

* Fixes #3169

* deal with CBONDS too

* test PATOMS

* Fixes #3175

* a bit of code simplification and test updates

still needs more testing

* more testing

* handle s-group hierarchy
also a couple of other changes in response to the review

* add forgotten test file

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

Successfully merging a pull request may close this issue.

2 participants