From: SourceForge.net <no...@so...> - 2009-03-13 13:59:44
|
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 Review 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-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 |