Attached is a patch which corrects the math in DistanceRestraint.
Currently if the interparticle distance is zero (or very near zero)
DistanceRestraint pretends that the particles are randomly placed in
near proximity, so that a random distance and derivatives are returned.
This is because the derivatives of the distance at zero are undefined
(the math is 0/0).
IMHO, this behavior is flawed, because the score of a given state should
always be the same - and the scoring function should be deterministic.
Any necessary randomization of the system should be introduced by
perturbing the system state before an optimization, not by having the
scoring function return random values. (The current behavior also makes
it impossible to introduce a distance restraint with zero mean, to force
two particles to be colocated.)
This patch returns a zero derivative for zero distance, which is at
least consistent with every molecular mechanics package I've seen.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle
Daniel Russel wrote:
>> They come and complain to me in person. And it is hard to object to a
>> change which isn't mentioned on the list, until much later when you
>> realize your code doesn't work any more.
> Recently? I haven't heard anyone complain in rather a while a month or so.
That's because they complain only to me.
> I agree that more should be on the list. I stopped emailing the list
> because I was just getting complaints about their being too much traffic
> on the list and no one else was contributing. Anyway, it should resume.
If people think there's too much traffic on the list, they don't have to
read the emails. But then they shouldn't complain either. ;) Most
importantly, the list emails are archived and publicly accessible, while
my personal email is not. So even someone that doesn't read the list can
be pointed to a URL if they complain about something.
>> You can have discussion before or discussion afterwards. It's your
>> choice. But just going ahead and writing the code is quite annoying if
>> it conflicts with code other people are writing in the meantime.
> Is there code outside of new_imp/ and the EM code that depends on it?
We have no way of telling, of course, because anyone is welcome to write
their own private modules. I don't _know_ of anybody doing that right
now, but we are just starting to add non-lab people to IMP, so I'm sure
it'll become more common.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle
Daniel Russel wrote:
>> You can't really expect changes to be committed rapidly if you make them
>> before discussing the implications, because that requires everyone to
>> agree with everything you do, which is not likely to be the case. ;)
> Well, I don't think we have ever had an objection from anyone else :-)
They come and complain to me in person. And it is hard to object to a
change which isn't mentioned on the list, until much later when you
realize your code doesn't work any more.
> Discussing thing beforehand doesn't help anyway, since it requires that
> I stop what I am doing and work on something else while the discussion
> occurs rather than just making the changes.
You can have discussion before or discussion afterwards. It's your
choice. But just going ahead and writing the code is quite annoying if
it conflicts with code other people are writing in the meantime.
> Sure. Much of the problem is the frequency of changes, not the latency
> of the commit time (although the unpredictability of the commit time is
> sometimes annoying). I am not always complaining about you. Just
> sometimes :-)
You can only predict the commit time if you can predict whether I'll
agree with your changes. One way to know that is to mention them on the
list before you make them.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle
Daniel Russel wrote:
> The problem is, I can't currently write very clean patches. I have to
> produce them from overlapping changes I have already made. If it were
> the case that the response time to the change were fast compared to the
> frequency of these changes, then they could be cleaner. It will happen
> eventually as things stabilize, just not quite yet.
You can't really expect changes to be committed rapidly if you make them
before discussing the implications, because that requires everyone to
agree with everything you do, which is not likely to be the case. ;) If
patches are clean, implement something we've agreed on doing, and are
consistent with the style guidelines, they can go in more or less
instantly. If they don't apply cleanly, make 'scons standards' complain,
or make huge changes to the interface without any kind of prior warning,
you can hardly be surprised if they get delayed a little.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle
Daniel Russel wrote:
[re: default argument for Restraint::show()]
>> I didn't see a unit test for this, but I guess I can write one for you
>> when I commit it.
> Well, I couldn't quite figure out anything useful to test especially as
> the stream gets dumped to /dev/null :-)
You can test that the method can be called, which is exactly what r634
does. I don't see a lot of value in testing what it prints, since it's
really only for human consumption anyway, but that can wait for a Python
file object to C++ stream interface.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle
Daniel Russel wrote:
> On Dec 13, 2007, at 11:41 AM, Ben Webb wrote:
>> Coverage is one thing, but a simple
>> unit test that directly accompanies a new piece of code helps to
>> document the intent of that code (e.g. in a submitted patch).
>>
>> But unit tests are not documentation
>> (sure, it'd be nice if they were readable enough to serve as simple
>> examples, but tests and examples are two rather different things).
>
> You above say that unit tests help to document the intent and below say
> that they are not documentation. I agree with the later and think trying
> to do it with the former just creates excess code with no purpose.
I don't see any inconsistency there - we're documenting different
things. Unit tests are not documentation in themselves, but do provide
some indication of the intent of a method which is useful in code review
(particularly if the accompanying description of the patch is lacking or
the doxygen comments are incomplete). I'm simply talking about
test-driven coding here. Documentation is great for saying "foo_method
takes a positive integer and returns its square, or RangeError on error"
but a unit test that invokes that method and asserts that RangeError is
thrown for a negative integer actually enforces that. That's not "excess
code with no purpose" because the documentation does not ensure that
happens.
> As a side note, having the test code in an unrelated patch is generally
> a bad idea,
I don't know why you say "as a side note", because that's the point I'm
making here.
> but separating out changes into clean patches is not always
> easy and in this case would have involved significantly more work.
Sure, which is why it's important to have a code review policy in mind
before writing such patches.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle
In interest of trying the new system, I'll dump my local changes back
to svn.
First, I made some minor cleanups to the decorator classes and added
the ability to get the key for each coordinate (x,y,z) from the
XYZDecorator.
It seemed sensible to me to come up with a default IMP code review
policy. Please take a look over it and if you have any issues with it,
think it needs to be clarified some more, etc., follow up to the list:
https://salilab.org/internal/manuals/imp/ch01s04.html
Text from that URL:
Code review
To ensure code quality, additions to the IMP kernel should be reviewed
by the other developers. In general, this means posting a patch with
some explanation of the changes (e.g. suitable for the source control
logs) to the <imp-dev(a)salilab.org> mailing list. This is mandatory for
changes that:
* Change the API (e.g. remove or rename existing methods or classes,
change the arguments taken by existing methods, change the behavior of
existing methods, change the return values or thrown exceptions of
existing methods).
* Remove or modify existing test cases.
For more minor changes, such as adding new tests, methods and/or classes
(which may change the ABI but not the API), documentation updates, etc.
review is not required but is recommended.
Patches should contain a related set of changes. For example, a patch
which adds a new method foo, a new testcase for foo, and some
documentation for the SpecialVector class, should be split into two
patches: one for the foo method and test, and the other for the
SpecialVector documentation.
New code in patches should also adhere to the coding style guidelines.
Rationale: Other developers may have external applications and their own
in-progress work which may be adversely affected by your changes. There
may also be disagreement about a change, so discussing it on the mailing
list first assures that a consensus is first reached, rather than other
developers later reverting the change. It is much easier to review a
patch if it is small and contains only relevant changes, and in addition
it can generally be applied to source control virtually unchanged if the
code is OK.
Ben
--
ben(a)salilab.org http://salilab.org/~ben/
"It is a capital mistake to theorize before one has data."
- Sir Arthur Conan Doyle