[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [IMP-dev] First significant code review




I don't like this patch, because it now requires you to use
ScoreFuncParams in some places and regular ScoreFuncs in others, and to
call the generator function yourself (even from Python). We should
decide on one or the other.

The reason I introduced the split, and, I think the reason for having both the Params and the SF itself are that some restraints simply do use a function, where as others generate a family of functions. They are conceptually different, and so I think, as long as there are two different class, the interfaces should reflect that. (the other reason is I didn't want to have to change BasicScoreFuncParams every time I added a new subclass).

(I will quite happily write the code, unless
Daniel really wants to, for the solution.)
I'll pass :-)

One suggestion: we have a hierarchy of ScoreFunc classes, and just have
the Restraint constructors create local copies to keep their own state
(we can just use the copy constructor for this).
I don't think I understand the proposal.

Two proposal (perhaps the first is the same as yours)

1) to have ScoreFunc which has a clone method and translate method (and perhaps a scale method). Then, a compound restraint can take one, clone it and shift the the origin to handle generating a family.

2) Have a factory per ScoreFunc which then takes a shift and a scale parameter and creates a corresponding score function. The factory can be easily generated with a macro.

As a side note, I propose we change the name to UnaryFunction because it is simply a function of one variable and I can see wanting functions of more than one. And having an abbreviation in the name is inconsistent. Also, the names mean and stdev have to go, since they don't make sense with statistical potentials.