[Cdk-devel] [ cdk-Patches-2675819 ] Patch to add a removeReaction to reactionSet

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Patches item #2675819, was opened at 2009-03-09 13:48
Message generated for change (Comment added) made by rajarshi
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2675819&group_id=20024

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: Needs Revision
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Stefan Kuhn (shk3)
Assigned to: Rajarshi Guha (rajarshi)
Summary: Patch to add a removeReaction to reactionSet

Initial Comment:
This adds a method public void removeReaction(IReaction relevantReaction); to IReactionSet, ReactionSet and has a test for it. It is for 1.2 branch

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-03-18 09:42

Message:
In testGetReactionByReactionID_IReactionSet_String can you add an assert
testing that you get back null for a non-existant ID. Otherwise looks good

----------------------------------------------------------------------

Comment By: Stefan Kuhn (shk3)
Date: 2009-03-18 09:27

Message:
Ok, I added a new patch which hopefully keeps the naming more consistent.

----------------------------------------------------------------------

Comment By: Stefan Kuhn (shk3)
Date: 2009-03-13 10:40

Message:
Using the IDs has some reason in JCP. I will rename the methods to be more
consistent.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-03-13 10:14

Message:
I second rajarshi's last comment...

Additionally, you could change containsByID() to
contains(IAtomContainerSet, String), so that both methods are indeed using
the same pattern, or have getRelevantReactionByID take an IReaction as
second parameter.

Actually, the last one should likely be called getReactionByID()... as
there is no 'relevant' involved... it either has the ID or not... it's not
like like the reaction with that ID seems 'relevant'...

+     * Removes all instance of a reaction from this reactionSet.

1. instanceS
2. either use {@link IReactionSet} or normal english like "reaction set".

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-03-13 09:59

Message:
Actually, in reference to my last comment, it seems that
getRelevantReaction should be renamed to getRelevantReactionByID to be
consistent with containsByID in AtomContainerSetManipulator

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-03-13 09:54

Message:
Regarding the new method in rxn set manipulator, why do you look for
reactions based on the id field rather than checking equality of
references? It seems that that is contrary to other cases of code where we
check for presence of objects based on references

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-03-10 13:01

Message:
re NNReactionSet, yes you're right I don't think there is anything to do
there.

re IMolecule, good question. My preference is to go with IAtomContainer.

I still don't know the purpose of IMolecule - it extends IAtomContainer
and adds nothing to it. So basically it's an empty layer of abstraction.
Why do we need it? Why do we have it?

----------------------------------------------------------------------

Comment By: Stefan Kuhn (shk3)
Date: 2009-03-10 12:41

Message:
Uploaded a new version of the patch, also including new method in
manipulators. Some comments:
- I can't see what I need to do in the NNReactionSet
- I added a method getRelevantReaction(IReactionSet reactionSet,
IAtomContainer atomContainer) to ReactionSetManipulator, bug found that
there is one with IMolecule instead of IAtomContainer. Is there a reason
for this? The method would be more universal with IAtomContainer and could
still be used with IMolecule. Or do I miss something?

----------------------------------------------------------------------

Comment By: Rajarshi Guha (rajarshi)
Date: 2009-03-10 11:41

Message:
In testRemoveReaction_IReaction() you should check that the correct
reaction is retained after removal

Also, the NNReactionSet should be updated accordingly

Also based on Egon's comment, the behavior regarding multiple instances of
a reaction in a ReactionSet should be specified:

* Are multiple instance of the same Reaction allowed?
* If so, removeReaction removes the first instance. Or should it remove
all?

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-03-10 10:14

Message:
The testRemoveReaction_IReaction() unit test does not test if the correct
reaction has been removed, and I would also suggest to assert there are two
reactions before the remove call.

First sentences in the JavaDoc must end with a period. (Please run
DocCheck against your code).

Did you check if one IReaction can occur multiple times in a IReactionSet
(I see nothing in the docs that it cannot)? If so, please define the
behavior of the method in such situations. For example, why should it not
remove all occurences of the IReaction, instead of just the first?

Implementation of the method for DebugReactionSet is missing.

----------------------------------------------------------------------

Comment By: Egon Willighagen (egonw)
Date: 2009-03-10 10:03

Message:
Rajarshi, can you please review this patch? (I'm doing so now...)

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2675819&group_id=20024