Here is my version of the harmonic. It removes the internal scaling factors since those only make sense given various assumptions about the system. Instead offers a function which allows you to compute the spring constant from a standard deviation (and a temperature) if you want those assumptions to hold. This way harmonic can be used to construct more complicated potentials as well as to mimic modeller/ charm if desired. No existing code meaningfully depended on the old behavior. The test code has been patched.
Daniel Russel wrote: > Here is my version of the harmonic. It removes the internal scaling > factors since those only make sense given various assumptions about the > system. Instead offers a function which allows you to compute the spring > constant from a standard deviation (and a temperature) if you want those > assumptions to hold. This way harmonic can be used to construct more > complicated potentials as well as to mimic modeller/charm if desired. No > existing code meaningfully depended on the old behavior. The test code > has been patched.
We talked about this before, as obviously anybody that uses the current Harmonic behavior will have to change all of their code. But since nobody appears to care either way (I don't recall seeing any responses to that thread) I will go ahead and commit Daniel's patch later today. You can then figure out how to fix your code. ;)
Ben
Daniel Russel wrote: > Here is my version of the harmonic. It removes the internal scaling > factors since those only make sense given various assumptions about the > system. Instead offers a function which allows you to compute the spring > constant from a standard deviation (and a temperature) if you want those > assumptions to hold.
Opinion differs on what the form of a harmonic function is - the CHARMM guys say the score is kx^2 (deriv 2kx) while the GROMOS guys use 0.5kx^2 (deriv kx). This patch is clearly wrong because it reports a score of kx^2 but a derivative of 0.5kx![*] The patch I committed as r471 uses the GROMOS convention, and explicitly states this in the headers so that there can be no confusion.
[*] Unfortunately this is missed by the unit tests right now because there is no way to overload return type in Python. (There is no way in C++ either - we overload the arguments instead - but SWIG converts both overloaded C++ functions to the same Python method.)
> No existing code meaningfully depended on the old behavior. The test code > has been patched.
Depends on what you mean by "meaningfully" I suppose. ;) But your patch breaks the Modeller tests, since they do assume standard deviation. But I fixed that in r471.
Ben
> [*] Unfortunately this is missed by the unit tests right now because > there is no way to overload return type in Python. (There is no way in > C++ either - we overload the arguments instead - but SWIG converts > both > overloaded C++ functions to the same Python method.) Swig handles return arguments via type maps, right? And handles overloaded functions. So is the problem that they can't coexist? Just wondering. It would be really nice to be able to run tests on unary functions.
> Depends on what you mean by "meaningfully" I suppose. ;) Well, test code doesn't count :-)
> But your patch > breaks the Modeller tests, since they do assume standard deviation. > But > I fixed that in r471.
I forgot to include the changes to pyext. Grumble.
Thanks for catching the deriv mismatch. That is what I get for moving things around while listening to a talk :-)
Daniel Russel wrote: >> [*] Unfortunately this is missed by the unit tests right now because >> there is no way to overload return type in Python. (There is no way in >> C++ either - we overload the arguments instead - but SWIG converts both >> overloaded C++ functions to the same Python method.) > Swig handles return arguments via type maps, right? And handles > overloaded functions. So is the problem that they can't coexist?
No - the problem is that in C++ we have two overloaded functions: Float Harmonic::operator()(Float feat); Float Harmonic::operator()(Float feat, Float &deriv); where &deriv is used for a second return value, since C++ only allows a single return value.
Usually, typemaps are set up in SWIG to map 'second return values' to real return values in Python, since Python allows multiple returns (but does not have the concept of references or pointers to primitive types). So these would be mapped to something like val = Harmonic(feat) (val, deriv) = Harmonic(feat) which wouldn't work of course, since you can't overload the return type.
(Actually, right now this is not done, and the second form still takes two arguments - but the second is an opaque SWIG "pointer-to-float" object which can't be used in Python.)
I propose mapping the second to a new Python method, i.e. (val, deriv) = Harmonic.evaluate_deriv(feat)
I can do this very easily without any changes to the C++ code. But maybe for consistency between C++ and Python it makes more sense to replace operator() with evaluate(feat) and evaluate_deriv(feat, &deriv) ?
> It would be really nice to be able to run tests on unary > functions.
Well, we already can, of course - just not on the derivatives.
Ben
> I can do this very easily without any changes to the C++ code. But > maybe > for consistency between C++ and Python it makes more sense to replace > operator() with evaluate(feat) and evaluate_deriv(feat, &deriv) ? I like having C++ and python the same and (*f_)(value) looks kind of silly anyway.
If it can be made to work easily in python, I would propose have the deriv version return a pair or tuple in C++ too.
>> It would be really nice to be able to run tests on unary >> functions. > > Well, we already can, of course - just not on the derivatives. Sure. But consistency between function and its derivative seems like the main nontrivial thing to check.
Daniel Russel wrote: >> I can do this very easily without any changes to the C++ code. But >> maybe >> for consistency between C++ and Python it makes more sense to replace >> operator() with evaluate(feat) and evaluate_deriv(feat, &deriv) ? > I like having C++ and python the same and (*f_)(value) looks kind of > silly anyway.
Agreed - so I put it in as r473.
> If it can be made to work easily in python, I would propose have the > deriv version return a pair or tuple in C++ too.
Yes, I had that in mind when I proposed it - now that the methods have different names, we can have different return types. But I don't know if SWIG has any typemaps for pair or tuple - if not it might be a bit awkward to make it work nicely in Python. It works just fine as is.
>>> It would be really nice to be able to run tests on unary >>> functions. >> Well, we already can, of course - just not on the derivatives. > Sure. But consistency between function and its derivative seems like > the main nontrivial thing to check.
Agreed.
Ben
SWIG wraps pairs so that you can use first and second and [0] and [1]. Unfortunately, the (a, b) = IMP.ParticlePair(pa, pb) doesn't work, so it doesn't seem to be quite as nice as a true python array.
Still, I think returning a pair is nicer than having different signatures.
On Mar 15, 2008, at 5:06 PM, Ben Webb wrote:
> Daniel Russel wrote: >>> I can do this very easily without any changes to the C++ code. But >>> maybe >>> for consistency between C++ and Python it makes more sense to >>> replace >>> operator() with evaluate(feat) and evaluate_deriv(feat, &deriv) ? >> I like having C++ and python the same and (*f_)(value) looks kind of >> silly anyway. > > Agreed - so I put it in as r473. > >> If it can be made to work easily in python, I would propose have the >> deriv version return a pair or tuple in C++ too. > > Yes, I had that in mind when I proposed it - now that the methods have > different names, we can have different return types. But I don't > know if > SWIG has any typemaps for pair or tuple - if not it might be a bit > awkward to make it work nicely in Python. It works just fine as is. > >>>> It would be really nice to be able to run tests on unary >>>> functions. >>> Well, we already can, of course - just not on the derivatives. >> Sure. But consistency between function and its derivative seems like >> the main nontrivial thing to check. > > Agreed. > > Ben > -- > ben@salilab.org http://salilab.org/~ben/ > "It is a capital mistake to theorize before one has data." > - Sir Arthur Conan Doyle > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
sorry for my ignorance of this discussion for a while. i presume most people will want to use imp in a probabilistic framework rather than doing simulations with physical form-fields. i.e. most people's scoring function will be a log(probability) rather an actual energy. for consistency it would be nice to call the scoring function with the according spatial observables, which are typically distances and the corresponding error. philosophically, i agree with with daniel that the name 'Harmonic' requires a spring constant. to make everybody happy, i would propose a new name for functions that call the harmonic functions, but have spatial parameters as an input. for example LogGauss, LogUpperDistanceGauss or whatever you agree with. is anybody be willing to contribute something like that to the kernel (i am an official application guy, so the kernel is 'tabu' for me)? i think the current workaround, i.e. introducing scaling parameters through the back-door, is a bit unsatisfactory and will confuse most users.
thanks
frido
On Mar 15, 2008, at 5:31 PM, Daniel Russel wrote: > SWIG wraps pairs so that you can use first and second and [0] and [1]. > Unfortunately, the > (a, b) = IMP.ParticlePair(pa, pb) > doesn't work, so it doesn't seem to be quite as nice as a true python > array. > > Still, I think returning a pair is nicer than having different > signatures. > > > On Mar 15, 2008, at 5:06 PM, Ben Webb wrote: > >> Daniel Russel wrote: >>>> I can do this very easily without any changes to the C++ code. But >>>> maybe >>>> for consistency between C++ and Python it makes more sense to >>>> replace >>>> operator() with evaluate(feat) and evaluate_deriv(feat, &deriv) ? >>> I like having C++ and python the same and (*f_)(value) looks kind of >>> silly anyway. >> >> Agreed - so I put it in as r473. >> >>> If it can be made to work easily in python, I would propose have the >>> deriv version return a pair or tuple in C++ too. >> >> Yes, I had that in mind when I proposed it - now that the methods >> have >> different names, we can have different return types. But I don't >> know if >> SWIG has any typemaps for pair or tuple - if not it might be a bit >> awkward to make it work nicely in Python. It works just fine as is. >> >>>>> It would be really nice to be able to run tests on unary >>>>> functions. >>>> Well, we already can, of course - just not on the derivatives. >>> Sure. But consistency between function and its derivative seems like >>> the main nontrivial thing to check. >> >> Agreed. >> >> Ben >> -- >> ben@salilab.org http://salilab.org/~ben/ >> "It is a capital mistake to theorize before one has data." >> - Sir Arthur Conan Doyle >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
> philosophically, i agree with with daniel that the name 'Harmonic' > requires a spring constant. > to make everybody happy, i would propose a new name for functions > that call the harmonic functions, but have spatial parameters as an > input. for example LogGauss, LogUpperDistanceGauss or whatever you > agree with. > is anybody be willing to contribute something like that to the > kernel (i am an official application guy, so the kernel is 'tabu' > for me)? i think the current workaround, i.e. introducing scaling > parameters through the back-door, is a bit unsatisfactory and will > confuse most users. I agree that adding a new set of scoring functions in the most satisfactory response. There is still the question of whether we should use straight standard deviation (so the energy is simply delta^2/ (2*sigma^2)) or have scaling factors as in the old harmonic.
On Mar 30, 2008, at 8:40 PM, Daniel Russel wrote: >> philosophically, i agree with with daniel that the name 'Harmonic' >> requires a spring constant. >> to make everybody happy, i would propose a new name for functions >> that call the harmonic functions, but have spatial parameters as an >> input. for example LogGauss, LogUpperDistanceGauss or whatever you >> agree with. >> is anybody be willing to contribute something like that to the >> kernel (i am an official application guy, so the kernel is 'tabu' >> for me)? i think the current workaround, i.e. introducing scaling >> parameters through the back-door, is a bit unsatisfactory and will >> confuse most users. > I agree that adding a new set of scoring functions in the most > satisfactory response. There is still the question of whether we > should use straight standard deviation (so the energy is simply > delta^2/ (2*sigma^2)) or have scaling factors as in the old harmonic. agreed. strictly speaking, the scaling is only relevant if one uses MD for optimization. probably, it is most convenient to include the scaling for most users. but both is fine with me, as long as it is documented. other opinions?
f
Friedrich Foerster wrote: > sorry for my ignorance of this discussion for a while. > i presume most people will want to use imp in a probabilistic > framework rather than doing simulations with physical form-fields.
Who knows? But we should allow for both.
> philosophically, i agree with with daniel that the name 'Harmonic' > requires a spring constant.
Yes, you are a bit late here - we changed that ages ago!
> to make everybody happy, i would propose a new name for functions that > call the harmonic functions, but have spatial parameters as an input. > for example LogGauss, LogUpperDistanceGauss or whatever you agree with.
Agreed - it's straightforward to introduce new unary functions. We can just stick them in as and when necessary.
> i think the current workaround, i.e. introducing scaling parameters > through the back-door, is a bit unsatisfactory and will confuse most > users.
I didn't realize that's what you were using scaling of restraints for, but I don't think that was ever their intended purpose. Scaling has been very useful for people tweaking their scoring functions - and I don't see that going away - but for your purposes it certainly sounds like new unary functions are a better bet.
Ben
participants (3)
-
Ben Webb -
Daniel Russel -
Friedrich Foerster