My 2p worth:
I am not a big fan of outright replacing the NumRotatableBonds
implementation (option 2). This is quite a popular descriptor which is
used in many ways (e.g. QSAR models, conformer generation, property
calculation, etc.). IF we are lucky (or skilful, or have had enough time),
we have tests written out for everything which will break as soon as soon
as we get different rotatable bonds count, and different results. We can
then revalidate our protocols using the new (strict) rotatable counts.
Perhaps we get better correlations/enrichments/AUCs etc ! Yeah!
On the other hand option (1), having two methods NumRotatableBonds() and
NumStrictRotatableBonds() will lead to some confusion. Greg has a point
about different people and/or libraries intermixing between the two.
Like Paul, I prefer option (3) - with the default behaviour giving the old
rotatable counts (not strict). This does not come for free either, as the
API becomes slightly less clean (and what to do in the future when, for
example, someone finds a non-SMARTS based way to do this -- add another
parameter?). Still I think this is the less of all evils.
Thanks Toby & Greg!
JP
On 31 January 2014 06:54, <paul.czodrow...@merckgroup.com> wrote:
> > I could add the new descriptor as Toby provided it. People are then
> > free to pick between NumRotatableBonds() and NumStrictRotatableBonds
> > (). This has the advantage of maintaining strict backwards
> > compatibility, but I could imagine it being confusing/irritating to
> > people using the code to have to choose between them (or, worse, using
> both).
> >
> > Another option is to just replace the current NumRotatableBonds()
> > SMARTS with the new one.
> > This loses backwards compatibility, but replaces NumRotableBonds()
> > with something more correct.
> >
> > Finally, I could take a hybrid approach: replace the default
> > NumRotatableBonds() with the new one, but add an extra argument that
> > allows the old one to be used.
>
> >
> > I'm leaning towards the second option. I'd normally go with the
> > third, but I almost view this as a bug fix for the rotatable bonds
> definition.
> >
> > Comments? suggestions? Other options?
>
> I like your idea of your hybrid approach which would mean backwards
> compatibility.
>
>
> paul
>
>
>
> This message and any attachment are confidential and may be privileged or
> otherwise protected from disclosure. If you are not the intended recipient,
> you must not copy this message or attachment or disclose the contents to
> any other person. If you have received this transmission in error, please
> notify the sender immediately and delete the message and any attachment
> from your system. Merck KGaA, Darmstadt, Germany and any of its
> subsidiaries do not accept liability for any omissions or errors in this
> message which may arise as a result of E-Mail-transmission or for damages
> resulting from any unauthorized changes of the content of this message and
> any attachment thereto. Merck KGaA, Darmstadt, Germany and any of its
> subsidiaries do not guarantee that this message is free of viruses and does
> not accept liability for any damages caused by any virus transmitted
> therewith.
>
> Click http://www.merckgroup.com/disclaimer to access the German, French,
> Spanish and Portuguese versions of this disclaimer.
>
>
> ------------------------------------------------------------------------------
> WatchGuard Dimension instantly turns raw network data into actionable
> security intelligence. It gives you real-time visual feedback on key
> security issues and trends. Skip the complicated setup - simply import
> a virtual appliance and go from zero to informed in seconds.
>
> http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk
> _______________________________________________
> Rdkit-discuss mailing list
> Rdkit-discuss@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
>
------------------------------------------------------------------------------
WatchGuard Dimension instantly turns raw network data into actionable
security intelligence. It gives you real-time visual feedback on key
security issues and trends. Skip the complicated setup - simply import
a virtual appliance and go from zero to informed in seconds.
http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk
_______________________________________________
Rdkit-discuss mailing list
Rdkit-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-discuss
