December 2007 - IMP-dev

Fix behavior of DistanceRestraint at near-zero distance
by Ben Webb 13 Dec '07

13 Dec '07

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

1 0

Re: [IMP-dev] First code review
by Ben Webb 13 Dec '07

13 Dec '07

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

1 0

Re: [IMP-dev] First code review
by Ben Webb 13 Dec '07

13 Dec '07

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

1 0

Re: [IMP-dev] First code review
by Ben Webb 13 Dec '07

13 Dec '07

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

1 0

Re: [IMP-dev] First significant code review
by Ben Webb 13 Dec '07

13 Dec '07

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

1 0

Re: [IMP-dev] First code review
by Ben Webb 13 Dec '07

13 Dec '07

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

1 0

First code review
by Daniel Russel 13 Dec '07

13 Dec '07

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.

2 9

Code review policy
by Ben Webb 12 Dec '07

12 Dec '07

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

2 1