[Biopython-dev] [Bug 2697] MaxEntropy calculate function assumes integer values for class and convergence criteria is hard coded

[bugzilla-daemon at portal.open-bio.org]

http://bugzilla.open-bio.org/show_bug.cgi?id=2697





------- Comment #5 from biopython-bugzilla at maubp.freeserve.co.uk  2008-12-14 08:17 EST -------
(In reply to comment #3)
> (In reply to comment #2)
> > No, you can change them in your own code - they are just module level
> > variables
> > ...
> > One might argue these should be *optional* arguments to the functions. 
> > However, your suggested change adds new *required* arguments, which is not a
> > backwards compatible API change.

Sorry - you *did* use optional arguments for the train function. I was
distracted by the private functions where the new arguments are required.

> I strongly disagree on this because a user should not have to read the module
> source code to find these module level global variables and what values these
> actually are. But this is not my code.

I'm not saying the current state of the code is elegant - just correcting your
factual error that the end user couldn't change these parameters.  They can.

(In reply to comment #4)
> I agree with Bruce that these variables should be arguments to the function,
> rather than module-level global variables. To keep the API backwards
> compatible, we can specify the current values for these variables as default
> values for these arguments. This will also make it easier for users that are
> not particularly interested in these variables.

This is what I was implying, although less clearly.

To be even more explicit, if we want to add these variables as arguments to the
functions then they should default to the existing upper case module level
variables.  We shouldn't remove or rename the module level variables in case
anyone was using them them in the way I illustrated in comment 2.

e.g.
def train(training_set, results, feature_fns, update_fn=None):

becomes something like this:

def train(training_set, results, feature_fns, update_fn=None,
          max_iis_iterations = MAX_IIS_ITERATIONS,
          iis_convere = IIS_CONVERGE,
          max_newton_iterations = MAX_NEWTON_ITERATIONS
          newton_coverage = NEWTON_CONVERGE):
#This function's code would then need updating to use
#local variable max_iis_iterations instead of the
#module level MAX_IIS_ITERATIONS.

Note this does NOT use uppercase argument names as in Bruce's original patch -
these would not be consistent with the rest of Biopython.

Peter


-- 
Configure bugmail: http://bugzilla.open-bio.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.