- IMP-dev - salilab.org

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

[Fwd: PDB lib]
by Ben Webb 19 Nov '07

19 Nov '07

Forwarded to the list for reference. -------- Original Message -------- Subject: PDB lib Date: Tue, 13 Nov 2007 16:06:24 -0800 From: Daniel Russel <drussel(a)gmail.com> Reply-To: drussel(a)salilab.org To: Ben Webb <ben(a)salilab.org> References: <3682E173-8139-44FE-8BB0-C2739807D71E(a)salilab.org> <473A0947.9030501(a)salilab.org> <473A09AD.2000603(a)gmail.com> <473A31BC.1060000(a)salilab.org> > > At any rate, this PDB reader stuff needs to be discussed on imp-dev > before we proceed. For example, what's wrong with the BALL stuff you > were playing with before? BALL is dead. No activity on email list. No response to bugs. No move to actually document their newest version even though it was released a year ago. I don't think we want to tie ourselves to it. Sure we can take it to IMP dev. No one else seems to care much :-) I have looked around and asked around and couldn't find any decent PDB readers (in C or C++) which are not buried in some huge project. > Why can't we link against this PDB library, rather than > cut-and-pasting thousands of lines of code? The nice thing about it is that it is small and simple and mine so we can just ship it along with IMP and not worry about dependencies, name collisions etc. I don't want people to have to get another library from somewhere else, hence my desire to put a copy into imp svn. Soon enough the lib will make it to fedora extras (whenever the next CGAL release is) so we could potentially just use that. > Should the kernel include a PDB reader anyway? It doesn't matter if it is in the kernel or a separate lib that is brought along, especially since we are using dynamic linking (so the library can be pulled in my libimp). Everyone seems to want a PDB reader, including me so we need one somewhere. > My concerns are 1. it'll be a maintenance nightmare to fork this PDB > library, Not at all. It doesn't actually need forking other than changing #include <CGAL/PDB to #include <IMP/PDB to avoid stomping on things. I really with there were some way to parameterize such things. It was written to be self-contained, I just got sloppy with the last few changes so I had to fix those this morning). > 2. it's LGPL so we can't include it in IMP without being forced to > adopt LGPL (only link against it) Why would we want to do more than link against it? Anyway, changing the license is easy. It was initially free, I just changed the license recently because much of the rest of CGAL is LGPL. > and 3. from a brief reading, it looks like a not-very-good PDB library > anyway (hard-coded atom names - what's with that?) Well, it is either that or use strings which pushes the checks to runtime rather than compile time. Adding to an enum and recompiling is trivial (and adding a constant externally works just as well for must purposes). Checking everywhere than an object falls in a small set of allowed strings is hard (especially if you can't specify that set of strings anywhere). BALL has hardcoded atoms for that matter (just a lot more of them :-) Anyway, I am a fan of it (as I should be :-) And since our backups are rather less than I thought, the ball code got lost with the constant rechecking out of IMP.

4 9

[Fwd: Re: [Fwd: PDB lib]]
by Ben Webb 16 Nov '07

16 Nov '07

Forwarding to the list: -------- Original Message -------- Subject: Re: [IMP-dev] [Fwd: PDB lib] Date: Fri, 16 Nov 2007 12:18:04 -0800 From: Eswar Narayanan <eswar(a)salilab.org> To: Ben Webb <ben(a)salilab.org> CC: imp-dev(a)salilab.org References: <473A42F7.4060005(a)salilab.org> <473BAF19.30207(a)salilab.org> <B3063E98-E0A6-4032-A70F-CF72419F8441(a)salilab.org> <473DE358.4000203(a)salilab.org> Somebody called me an expert, so I'm compelled to reply! I must confess though that I haven't been paying attention to the discussions before this and couldn't locate the origin of this PDB thread. So I'm probably missing some critical information here - for instance, I have no clue why so much is being discussed about reading and interpreting a PDB file. Absolutely nothing in the PDB file is believable (I can give you any number of examples to prove this). So by definition, if someone claims they have the ultimate reader, I think that person should seriously consider taking astrology as a vocation. But my two cents: I think the PDB parser of Modeller is already quite good and could be made better with just a few tweaks. More than anything else, I think we completely understand where the problems are. So even if it comes to recoding it from scratch (if licensing is an issue), in the long run, it is probably much easier and faster than tinkering with someone else's code. Ben and I have already had long email conversations on some of the limitations of the Modeller implementation and at least in principle know how to fix it. And even if Ben pokes fun at it, it has withstood the test of time for close to two decades. Is there a reason you don't want to use the PDB reader from Modeller? E. On Nov 16, 2007, at 10:37 AM, Ben Webb wrote: > Daniel Russel wrote: >> I think Ben and I have a disagreement about the significance of >> picking >> a PDB reader at this point. As I see it, any PDB reading should be >> hidden behind some very general interface (I have a proposal using >> the >> MolecularHierarchyDecorator). > > Agreed. We can certainly use whichever PDB reader you're happy with in > the short term. But I have a lot of experience with reading dodgy PDB > files (although Eashwar is probably the lab expert in this regard) so > know there's a lot of corner cases to worry about, and I don't relish > fixing all of these again with a new PDB reader. And that's before we > start worrying about heterogens. > >> I picked my pdb reader since it is small and so can be stuck in the >> with >> rest of imp so that no one has to worry about installing external >> libraries and it does what I want, namely give me a hierarchy for >> proteins and a bond information. > > We're talking about two different things here. You want to distribute > your PDB reader with IMP. I don't want to include the code in IMP SVN. > The two issues are orthogonal; if you really want to bundle the > code, we > can do it at makedist time (for tarball releases) or we can use an SVN > externals definition if you want it for SVN users. For the latter, > just > point me to the URL of your SVN repository (presumably this is a path > within the CGAL SVN, or if you prefer I can make a repository for your > PDB reader at svn.salilab.org) > >> the one Frido sent- I don't see how to get bonds out of it, but >> otherwise fine. The documentation really sucks, so I might be missing >> something. > > How are you "getting bonds" out of a PDB file? PDB files don't provide > that information. (The most you can get is the CONECT and SSBOND > records.) For that you generally need a description of the topology, > which is generally covered by the topology file portion of most MM > forcefields. I really hope this stuff isn't hard-coded, because that > would really have trouble with patches and other residue modifications > (think covalently-bonded ligands, acetylated termini, disulfides, > MSE's, > cyclic proteins, nucleic acids, etc.). > >>> Hao's project absolutely requires HETATMs, for example. And I >>> don't share your concern for runtime checks, since PDB reading is >>> not >>> performance-critical. >> My concern on checks was not for efficiency, it was for correctness. >> Depending on strings is poor as capitalization or abbreviation errors >> don't easily get caught. > > I didn't mean you wouldn't have actual atom type objects, just that > they > shouldn't be hardcoded. For example, Modeller reads a set of residue > types from its parameter files at runtime, and after that maps every > residue type in the PDB file from the string to an integer residue > type. > Unknown residue types result in a warning, and the generation of a new > integer residue type at runtime. You could of course use Residue > objects > rather than integer types. > > Ben > -- > ben(a)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(a)salilab.org > https://salilab.org/mailman/listinfo/imp-dev -- ben(a)salilab.org http://salilab.org/~ben/ "It is a capital mistake to theorize before one has data." - Sir Arthur Conan Doyle

2 2

score function weights
by Friedrich Foerster 07 Nov '07

07 Nov '07

4 19

Remaining open questions
by Daniel Russel 07 Nov '07

07 Nov '07

1) Hiding the Index classes and making attributes accessed only through the Particles themselves rather than through the ModelData. This simplifies things a lot and we should be able to optimize away most of the overhead later if needed. 2) Decorator classes I vote yes for both :-) And (1) should be done sooner rather than later and effects the DerivativeAccumulator.

1 0

Faking objects
by Daniel Russel 07 Nov '07

07 Nov '07

To elaborate further on my proposal from last night, a particle bundle would look like the following (incorporating some unrelated proposed changes) class XYZBundle { static FloatKey kx_, ky_, kz_; public: XYZBundle(Particle*p): p_(p){} static bool is_instance(Particle *p) { return p->has_attribute(kx_) && p->has_attribute(ky_) && p- >has_attribute(kz_); } typedef std::auto_ptr<XYZBundle> Pointer; Pointer cast(Pointer *p) { if (!is_instance(p)) return Pointer(); else return Pointer(new XYZBundle(p)); } // either static XYZBundle::Pointer initialize(Particle *p) { p->add_attribute(kx_, 0); p->add_attribute(ky_, 0); p->add_attribute(kz_, 0); return Pointer(new XYZBundle(p)); } // or static XYZBundle::Pointer initialize(Particle *p, Float x, Float y, Float z) { p->add_attribute(kx_, x); p->add_attribute(ky_, y); p->add_attribute(kz_, z); return Pointer(new XYZBundle(p)); } Float get_x() const { return p->get_attribute(kx_); } void set_x(Float x) const { return p->set_attribute(kx_, x); } // etc }; The user class XYZBundle::Pointer xyzp= XYZBundle::initialize(p) to add the right fields or XYZBundle::Pointer xyzp= XYZBundle::cast(p) to use. I am a bit uncomfortable about the faking aspect of the design especially without working reference counting. Since Bundles don't need to get passed back and forth from python, we could make the reference counting work. It might be better to just allocate Bundles on the stack. This removes a memory management issue at the cost of eliminating the possibility of using a NULL pointer to signify failure. So things would have to look more like class XYZBundle { static FloatKey kx_, ky_, kz_; public: XYZBundle(Particle*p): p_(p){if (!is_instance(p)) p_=NULL;} bool get_is_valid() const {return p_ != NULL;} static bool is_instance(Particle *p) { return p->has_attribute(kx_) && p->has_attribute(ky_) && p- >has_attribute(kz_); } // either static XYZBundle initialize(Particle *p) { IMP_assert(!is_instance(p)); p->add_attribute(kx_, 0); p->add_attribute(ky_, 0); p->add_attribute(kz_, 0); return XYZBundle(p); } // or static XYZBundle::Pointer initialize(Particle *p, Float x, Float y, Float z) { IMP_assert(!is_instance(p)); p->add_attribute(kx_, x); p->add_attribute(ky_, y); p->add_attribute(kz_, z); return XYZBundle(p); } Float get_x() const { return p->get_attribute(kx_); } void set_x(Float x) const { return p->set_attribute(kx_, x); } // etc }; the the user calls XYZBundle xyzb(p); and xyzb.is_valid() to manipulate a Particle and XYZBundle xyzb= XYZBundle::initialize(p); to initialize.

1 1

Death to objects
by Daniel Russel 06 Nov '07

06 Nov '07

A possible fatal blow for objects came up when talking with Jeremy. Lets say we load a PDB file. It creates a bunch of Particles, some of which are AtomParticles, some ResidueParticles etc. Now it happens I am doing coarse grained modeling, so I only care about the Residues and want to approximate them by a sphere. I have no way of adding a radius attribute to the already existing particle. I have to copy the particle to a new ResidueWithRadiusParticle (and delete the old one) or add a new particle linked to the existing one. Deleting the old residue would break all existing restraints which use the particle.

2 2

Objecting to objects
by Daniel Russel 06 Nov '07

06 Nov '07

I think I have a proposal I like over the other ones: See below or https://salilab.org/internal/wiki/IMP/Particle_objects_or_table - keep the attributes identified by strings - bundle attributes into groups which are presented along with a set of helper functions--this can be done in Doxygen easily - using a string which internally merges its representation (see the web page) - get and set values for the attributes through the Particle, rather than through the attribute table This approach is completely flexible. Users are encouraged to use the helper functions since they are bundled up with the attribute names themselves. Getting an attribute involves a search through a list of integers before fetching it from the table--so not as good as the objects case, but not horrible either and I'll bet we can make the compiler build it to something that is the same as the current Index based version via inlining. There are no changes to IO or the XML or the web stuff. Restraints have to get rid of XIndex classes.

1 0