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

Re: [IMP-dev] First significant code review



Daniel Russel wrote:
DistanceRestraint now takes a pair of ParticleIndexes and a ScoreFunc which it then owns.

OK, so the ScoreFunc change should be a separate patch, because it is a large change. And first we need to decide what the proper solution is, because your patch appears to only address part of the issues.

So what is a ScoreFunc? It is essentially what in the Modeller world is called a math form - given some value, it uses its own state (e.g. mean and standard deviation) to return a score. IMP has harmonic and upper/lower bound ScoreFuncs which are essentially identical to the Modeller equivalents.

Some restraints (e.g. DistanceRestraint) internally keep a pointer to a ScoreFunc object and use it in the score calculation.

Originally, Bret had DistanceRestraint take a ScoreFunc, a mean, and a standard deviation. I complained about this because it makes things like DOPE impossible - because those distance restraints do not have means or standard deviations, just a cubic spline. So he replaced ScoreFunc with ScoreFuncParams, which is a generator that can make ScoreFunc objects internally. For example, something like the ExclusionVolumeRestraint goes through all pairs of particles and builds distance restraints for each pair, calling ScoreFuncParams::set_mean to set the mean for each DistanceRestraint generated.

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. (I will quite happily write the code, unless Daniel really wants to, for the solution.)

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'd like to hear people's thoughts on this, because other scoring functions (e.g. Lennard-Jones style 6-12 potential) are on my immediate todo list.

 I also removed the constructor with a passed radius
as that does add any power and adds another path which has to be tested and maintained.

Sure, I can commit that.

ExclusionRestraint takes a list or lists of ParticleIndexes (instead of ints).

Do others have any opinions on this? If people hate ParticleIndexes, now would be the time to say so - otherwise I'll commit this part of Daniel's patch too.

Restraint::show now has a default argument so it can be called from python.

I didn't see a unit test for this, but I guess I can write one for you when I commit it.

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