Sorry, hit send prematurely.
I'd define a separate function FindMCSWithTimeout for the timeout parameter
unless a majority of users will be concerned with this method's running
time, but I'm not familiar with the algorithm's details.
Best,
James
On Sat, Sep 1, 2012 at 8:57 AM, James Swetnam <jswet...@gmail.com> wrote:
> Some pendantic and unsolicited API critiques, but as a user I figured I
> should chime in.
>
> Agree with Greg's assertion to rename to FindMCS.
>
> def MCS(mols, min_num_atoms=2,
> maximize = Default.maximize,
> // Rename Default to something like MCSParams, and have a factory
> function returning a DefaultMCSParams I assume this is the command pattern?
>
> atom_compare = Default.atom_compare,
> bond_compare = Default.bond_compare,
> match_valences = Default.match_valences,
> ring_matches_ring_only = False,
> complete_rings_only = False,
> timeout=Default.timeout,
> // Could you remove timeout? This is OpenBabel likeIt's orthogonal to
> the method itself. You could define an additional method with
> times=None,
>
> verbose=False,
> // Logging should really be separated from API methods. You should be
> using the python logging framework
> <http://docs.python.org/library/logging.html>such that users
> // can modify these globally and not through the API
> verbose_delay=1.0,
> // Also remove from this function
> ):
>
>
> On Fri, Aug 31, 2012 at 8:37 PM, Greg Landrum <greg.land...@gmail.com>wrote:
>
>> On Sat, Sep 1, 2012 at 1:28 AM, Andrew Dalke <da...@dalkescientific.com>
>> wrote:
>> > I've started to add the fmcs MCS search code to RDKit.
>> > By that I mean it does not include the command-line driver
>> > which is part of the fmcs distribution; just the MCS search code.
>>
>> I would have no objection to the command-line driver being there in a
>> separate file in the package; it could well be useful to people.
>>
>> > The interface to it is a single function which takes these
>> > parameters.
>> >
>> > def MCS(mols, min_num_atoms=2,
>> > maximize = Default.maximize,
>> > atom_compare = Default.atom_compare,
>> > bond_compare = Default.bond_compare,
>> > match_valences = Default.match_valences,
>> > ring_matches_ring_only = False,
>> > complete_rings_only = False,
>> > timeout=Default.timeout,
>> > times=None,
>> > verbose=False,
>> > verbose_delay=1.0,
>> > ):
>>
>> Any objection to calling it FindMCS?
>>
>> >
>> > The steps I've done so far are:
>> > - rename all other functions and classes to use the
>> > "_" prefix to indicate that they are internal
>> > - remove the command-line driver code
>> > - removed all code uses of the word 'fmcs'
>> >
>> > The steps still to do are:
>> > - use the right module name
>> > - reformat the doc strings
>> > - make it build properly
>> > - add basic tests
>> > - add the above to SVN and commit to SF
>> >
>> > I have some questions about those steps.
>> >
>> > 1) What is the right module name? Greg and I talked
>> > about it a couple of months ago. I can't find the
>> > paper where I wrote it down.
>> >
>> > Right now it is "rdkit.Chem.MCS", so that the actual
>> > function to call is "rdkit.Chem.MCS.MCS".
>>
>> MCS is fine with me, the function would then be rdkit.Chem.MCS.FindMCS
>> assuming that you agree with my function renaming suggestion.
>>
>> > 2) Is there any other documentation I should update
>> > other than the docstrings?
>>
>> It would be nice to have something about it in the Getting Started
>> guide, but I can do that.
>>
>> >
>> > I'll have more questions which I get to the build system.
>> >
>> >
>> > I'm not sure of the long-term coordination between
>> > the RDKit and fmcs code bases. Greg would like to
>> > push more of the RDKit code into C++ while I would
>> > like fmcs to be portable across other toolkits.
>>
>> Since the rest of the list haven't been involved in these discussions,
>> here's my motivation for that:
>> having the MCS code in C++ increases the impact of the code: it allows
>> it to be used from the cartridge, in the knime nodes, from other Java
>> programs, and from C++ itself. There may also be performance benefits
>> (I say "may" because I know that Andrew writes really good Python
>> code), but those are at most a secondary motivation
>>
>> -greg
>>
>>
>> ------------------------------------------------------------------------------
>> Live Security Virtual Conference
>> Exclusive live event will cover all the ways today's security and
>> threat landscape has changed and how IT managers can respond. Discussions
>> will include endpoint security, mobile security and the latest in malware
>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>> _______________________________________________
>> Rdkit-devel mailing list
>> Rdkit-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/rdkit-devel
>>
>
>
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Rdkit-devel mailing list
Rdkit-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rdkit-devel
