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

Re: [IMP-dev] First significant code review



Daniel Russel wrote:
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.

Yes, this is true. For example, a DistanceRestraint uses a 'raw'
ScoreFunc, while an ExclusionVolumeRestraint uses a ScoreFunc generator
(to generate a separate ScoreFunc for each DistanceRestraint). But don't
you think the average user will be confused by this distinction? I have
no problem with making it internally.

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)

Yes, the first is essentially the same as my proposal.

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.

There'd have to be a few more methods than that, though - for example an atomistic statistical potential would need to know the atom types of the two atoms it acts on, so there'd need to be a method to provide it with the two Particle pointers or indexes.

As a side note, I propose we change the name to UnaryFunction

That's a rather vague name IMHO, and doesn't say anything about its use as a scoring function... you may as well just call it MathForm (although the comments in ScoreFunc suggest that's what it was called originally anyway). Why don't you like MathForm? We could have UnaryMathForm if you prefer. ;)

because it is simply a function of one variable and I can see wanting functions of more than one.

That's not a huge stretch of the imagination, because we already have functions of more than one variable in Modeller (ND spline, Mike's MDG, multi bionormal). It's a shame that this was thrown away in the original IMP design, because now we've got to stick it back in. I guess you're arguing for a different base class for these functions, since the operator() call signature would be different? (std::vector<Float> rather than Float, I guess.)

Also, the names mean and stdev have to go, since they don't make sense with statistical potentials.

That's certainly true for statistical potentials, but most of the restraints that currently generate DistanceRestraints (e.g. ExclusionVolume) explicitly build restraints for a minimum or mean distance. So there'd need to at least be a ScoreFunc class that supported mean/stdev, that we can derive from, yes?

	Ben
--
                      http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
	- Sir Arthur Conan Doyle