Patch/qsarmol nomod #407
Patch/qsarmol nomod #407
Conversation
… why cloning is bad) - fix the clone and use it in this descriptor.
| @@ -115,7 +114,7 @@ public IMonomer getMonomer(String cName) { | |||
| /** | |||
| * Returns a collection of the names of all <code>Monomer</code>s in this | |||
| * polymer. | |||
| * | |||
| *f | |||
| @@ -64,7 +63,7 @@ | |||
| */ | |||
| public Polymer() { | |||
| super(); | |||
| monomers = new Hashtable<String, IMonomer>(); | |||
| monomers = new HashMap<>(); | |||
There was a problem hiding this comment.
This is new Java magic, right? It takes the typing automatically from the type of the 'monomers' variable definition, correct? (I guess a rhetorical question :)
| @@ -114,6 +114,8 @@ public DescriptorValue calculate(IAtomContainer atomContainer) { | |||
| throw new IllegalStateException("descriptor is not initalised, invoke 'initalise' first"); | |||
| } | |||
|
|
|||
| atomContainer = clone(atomContainer); // don't mod original | |||
There was a problem hiding this comment.
Yes, very much agreed, this is not how it should be. In the past I have changed code to not have to do this. But as a workaround this will do for now.
There was a problem hiding this comment.
(But that explains why you will find that flag copying and restoring code around :)
| } | ||
|
|
||
| @Test | ||
| public void descriptorDoesNotChangeFlags() throws CDKException { |
|
@johnmay, I guess you will want to fix the one type, but that's the only thing I suggest to change. |
|
done |
|
I propose an idea.
I consider it does not break existing codes and may improve performance when we need various properties of one molecule. |
|
Possible, in general this is similar to what RDKit and I think Open Babel does, I know that Noel for example would like to remove it. For example they have flags like 'hasRingPerception' done, this extra book keeping is problematic and less flexible. However in a reduced form just for the QSAR it could be useful. I would however set the flag/option on the descriptors. Like the ones that allow/skip aromaticity perception. Then in the engine you can set this globally such that it's configured once and then never agin. John |
I don't like this but it fits with the current design. Note that if you don't assign atom types to the benzene i used in the test... then aromaticity won't get added and it looks okay. As shown here it's not really okay. I would argue this is inconsistent - there require atom types must be set before being called.
Also interesting that the AcidicGroupCount vs BasicGroupCount - one has an aromaticity option the other does not.