Hi,

gilleain torrance wrote:
Hi,

On 1) I think : yes, go ahead. As you say, it only enforces a
situation that would have failed anyway, and makes things clearer.
  
IMHO , fine for a quick patch.
On 2) I used to think yes, now I think no :) Since someone (egon?) has
gone to the effort of writing a custom container for molecules, it
seems pointless to throw it away in favour of a more generic
List<IMolecule>. There is also the slight benefit that you mention of
having an ID. And properties, I guess.
  
What about getting the best of two worlds - introducing smth like

IChemObject<T>  implements List<T>  ?

Regards,
Nina
gilleain

On Thu, Apr 30, 2009 at 8:01 AM, Egon Willighagen
<egon.willighagen@gmail.com> wrote:
  
Hi all,

Stefan brought recently up a ClassCastException problem in the new
JChemPaint code, caused by the fact that IMoleculeSet can contain a
mere IAtomContainer too, while the getMolecule() methods assumes only
IMolecule's are stored.

A work around is:

    public org.openscience.cdk.interfaces.IMolecule getMolecule(int number)
    {
-        return (Molecule)super.getAtomContainer(number);
+        IAtomContainer container = super.getAtomContainer(number);
+        if (container instanceof IMolecule) return (IMolecule)container;
+        return container.getBuilder().newMolecule(container);
    }


but has the disadvantage that the IMolecule implementation constructor
may not faithfully copy all properties... very likely giving
downstream problems.

What Stefan and I discussed was to solve the conflict in the
IMoleculeSet to enfore that only IMolecule's can be stored (there is
always the IAtomContainer which allows storing both), but since
Rajarshi also questioned to need of IMoleculeSet and actually also
IAtomContainerSet, I suggested to discuss this on the CDK Workshop...
that did not happen, which is why I bring it up now...

I like to split up this discussion into: 1. fix IMoleculeSet, 2. drop
IMoleculeSet completely (or not). Where 1 is targeted at fixing the
1.2.x series, while the second is for CDK 2.0 (where likely other API
changes will happen too).

For 1, I suggest we implement the enforcement, throwing
IllegalArgumentExceptions if IMoleculeSet.addAtomContainer() is called
with an IAtomContainer not instanceof IMolecule. The exception will
happen in situations only when it would also cause an
ClassCastException in getMolecule, but it is a functional change,
though clean-up and actual fix.

So, the involved patch looks like:

+    /**
+     * Adds an atomContainer to this container.
+     *
+     * @param  atomContainer  The IMolecule to be added to this container
+     * @throws IllegalArgumentException when the passed IAtomContainer is not
+     *         an IMolecule.
+     */
+    public void addAtomContainer(IAtomContainer atomContainer) {
+        if (!(atomContainer instanceof IMolecule))
+            throw new IllegalArgumentException(
+                "Only IMolecule's can be stored in an IMoleculeSet"
+            );
+        super.addAtomContainer(atomContainer);
+    }
+

Please do reply your opinion on this fix.

Regarding 2: discussion is open: IMoleculeSet or List<IMolecule>? Drop
the first as being redundant, or do we really need and use
IMoleculeSet.getID() and friends inherited from IChemObject?

Egon

--
Post-doc @ Uppsala University
http://chem-bla-ics.blogspot.com/

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations
Conference from O'Reilly Media. Velocity features a full day of
expert-led, hands-on workshops and two days of sessions from industry
leaders in dedicated Performance & Operations tracks. Use code vel09scf
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf
_______________________________________________
Cdk-devel mailing list
Cdk-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cdk-devel

    

------------------------------------------------------------------------------
Register Now & Save for Velocity, the Web Performance & Operations 
Conference from O'Reilly Media. Velocity features a full day of 
expert-led, hands-on workshops and two days of sessions from industry 
leaders in dedicated Performance & Operations tracks. Use code vel09scf 
and Save an extra 15% before 5/3. http://p.sf.net/sfu/velocityconf
_______________________________________________
Cdk-devel mailing list
Cdk-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cdk-devel