- IMP-dev - salilab.org

Comments on transformation stuff
by Daniel Russel 11 Nov '08

11 Nov '08

Rotation3D: - the protected methods should probably be private as nothing is likely to inherit from Rotation3D and doxygen skips documenting private methods and the methods are not general purpose - it would be nice to be able to initialize from a vector and an angle - the current constructor should probably say Euler angle somewhere in its docs. Matrix3D: - why are there the rotation methods there? A cleaner interface would probably be to have a method is_rotation() and another get_rotation() so you can check is is close to a rotation and then get the rotation3D object out directly. Having to go through intermediates of some sort is messy. - it needs an operator[] which returns a vector3D (with appropriate bounds checking and all that). - I would suggest using \brief to define multi-line brief comments ParticleFunction: - Why does ParticleFunction take Particles? If you want that it should be ParticlesFunction, but it seems to me what we really want is for it to only take one particle and then use apply methods if we want to apply it to more than one. Otherwise we will be creating Particles with one Particle everywhere. - apply should be pure virtual so that you can tell if you got the name or signature wrong when overriding it. - at least one of the methods needs to be defined in a .cpp to get linking and casting right when using multiple libraries

2 2

minor breaking change
by Daniel Russel 30 Oct '08

30 Oct '08

In order to make the names more consistent XYZDecorator::get_vector is now XYZDecorator::get_coordinates (since the set version was set_coordinates).

1 0

Documentation of new IMP policies
by Ben Webb 21 Oct '08

21 Oct '08

Please see http://salilab.org/~ben/impdoc/ in particular http://salilab.org/~ben/impdoc/ch01s05.html (this should make it through the build system tomorrow). This lays out the policies previously discussed last month for the IMP reorg. If everybody is still happy with this, I will go in and open up the access to the core and misc modules. 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 1

Re: [IMP-dev] [IMP-commits] r770 - trunk/doc
by Daniel Russel 20 Oct '08

20 Oct '08

Looks good except I think that sending paches to impdev is not a great way to raise changes for discussion. An English description and proposed function signatures (if non trivial functions are propsed) is probably more useful ((and both patches and English is a bit much) On Oct 20, 2008, at 12:07 AM, Notification of IMP commits <imp-commits(a)salilab.org > wrote: > Author: ben(a)SALILAB.ORG > Date: 2008-10-20 00:07:58 -0700 (Mon, 20 Oct 2008) > New Revision: 770 > > Modified: > trunk/doc/usage.xml > Log: > Document > - commit writes for kernel and modules > - modules outside of the IMP repository > - namespace macros > - code maturity > - finding and fixing bugs > > > Modified: trunk/doc/usage.xml > =================================================================== > --- trunk/doc/usage.xml 2008-10-20 05:38:34 UTC (rev 769) > +++ trunk/doc/usage.xml 2008-10-20 07:07:58 UTC (rev 770) > @@ -152,11 +152,21 @@ > There are also two "general purpose" modules: the <filename>misc</ > filename> > module, which contains miscellaneous features which are > generally <emphasis>not</emphasis> used by other modules, and > -the <filename>core</filename> module, which contains general- > purpose features > -and upon which many other modules depend. The <filename>misc</ > filename> module > -can be used for features that do not neatly fit into a specialized > module, > -such as code that implements proposed new methodologies. > +the <filename>core</filename> module, which contains features > expected to be > +used by almost everybody, and upon which many other modules depend. > +The <filename>misc</filename> module can be used for features that > do not > +neatly fit into a specialized module, such as code that implements > proposed > +new methodologies. > </para> > + > +<para> > +It is also possible to create your IMP modules outside of IMP > itself. This > +is ideal if you have different licensing requirements, your code is > very > +experimental, or you do not want to share your source code, for > example. > +Of course, if you go down this route, you must do your own book > keeping to > +make sure that the IMP kernel and any modules you depend on are > correctly built > +before your own. > +</para> > </sect1> > > <sect1 id="style"> > @@ -233,7 +243,7 @@ > <sect3> > <title>Exceptions</title> > <para>Exceptions are classes, so name as for classes, above. Do not > use > -exception specfications, but do document the possible exceptions > thrown by > +exception specifications, but do document the possible exceptions > thrown by > a function with the Doxygen <command>\exception</command> command. > </para> > > @@ -297,6 +307,11 @@ > Avoids namespace pollution, and removes any ambiguity. > </para> > > +<para>Use the provided macros such as <code>IMP_BEGIN_NAMESPACE</ > code> and > +<code>IMP_END_NAMESPACE</code> to begin and end the namespace for > the kernel > +or a module. > +</para> > + > </sect2> > > <sect2> > @@ -347,14 +362,50 @@ > > </sect1> > > -<sect1 id="codereview"> > -<title>Code review</title> > +<sect1 id="adding-code"> > +<title>Adding code to IMP</title> > > -<para>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 > -<email>imp-dev(a)salilab.org</email> mailing list. This is mandatory > for > -changes that:</para> > +<sect2 id="choose-module"> > +<title>Choosing a module for your code</title> > + > +<para> > +By far the easiest way to contribute to IMP is to create a module > for your > +specific problem domain. For example, restraints and optimizers > specific to > +ligands could go in a <filename>ligand</filename> module. Please > contact Ben > +to have a new module created. If you feel your code doesn't belong > in its > +own module, you can add it to the <filename>misc</filename> module > instead. > +If you feel your new functionality is of use to everybody and > should live in the > +<filename>core</filename> module, you should request this on the > IMP mailing > +list and have it approved by Ben, who has the final say on where > code should > +end up. This is partly to prevent the <filename>core</filename> > module from > +becoming too bloated with specialized code. > +</para> > + > +<para> > +Code should generally <emphasis>not</emphasis> be moved from one > +module to another, unless there is an exceptionally good reason to > do so. If > +for some reason you do want to move things between modules, ask Ben > to do it. > +</para> > +</sect2> > + > +<sect2 id="define-interfaces"> > +<title>Defining your interfaces</title> > + > +<para> > +Before you sit down and write your new code or change existing > code, you > +should think about the interfaces. These are the classes and their > methods, > +without any implementation. It is recommended that you discuss these > +interfaces on the <email>imp-dev(a)salilab.org</email> mailing list > before you > +proceed with implementation, so that you do not duplicate work, > others can > +make valuable suggestions or point out problems, and people are > made aware of > +your contribution. If you want to add to or change the interfaces > in the > +kernel or the <filename>core</filename> module, this discussion is > +mandatory, since potentially everybody could be affected by your > changes. Any > +issues raised by users with changes to the kernel or > <filename>core</filename> > +must be resolved to their satisfaction before the changes are made. > For clarity, > +the following should be considered changes that warrant discussion: > +</para> > + > <itemizedlist> > <listitem><para>Change the API (e.g. remove or rename existing > methods or > classes, change the arguments taken by existing methods, change the > behavior > @@ -364,10 +415,25 @@ > <listitem><para>Remove or modify existing test cases.</para></ > listitem> > </itemizedlist> > > -<para>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 <emphasis>required</emphasis> but is recommended.</ > para> > +<para> > +Conversely, changes in internal implementation or documentation to > any module, > +or changes in the interfaces of your own modules or the > +<filename>misc</filename> module, do not need to be announced, and > neither does > +the addition of new methods to existing classes, although it may be > helpful > +to tell people about such changes. > +</para> > > +</sect2> > + > +<sect2 id="patches"> > +<title>Creating a patch</title> > + > +<para> > +Generally speaking, to add code to IMP, your changes should be made > to your > +checkout of the IMP repository, and then a patch generated by using > the > +<command>svn di</command> command. > +</para> > + > <para>Patches should contain a related set of changes. For example, > a patch > which adds a new method <methodname>foo</methodname>, a new testcase > for > <methodname>foo</methodname>, and some documentation for the > @@ -384,17 +450,97 @@ > other effects is better than nothing.)</para> > > <para><emphasis>Rationale:</emphasis> > -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. Test cases also help to > document the > +It is much easier for others to understand your patch's changes if > it is small > +and contains only relevant changes. Test cases also help to > document the > intent of code changes, as well as making sure that the new > additions are not > accidentally broken in future. > </para> > > +</sect2> > + > +<sect2 id="commit-patch"> > +<title>Committing a patch</title> > + > +<para> > +Once your patch is complete and you have run the testcases to make > sure you > +didn't break anything, and reviewed the patch to check your work, > it can be > +committed to the source control system, provided that any interface > changes > +have <link linkend="define-interfaces">been discussed</link> if > required. > +How this is done depends on whether you have commit rights for the > affected > +module(s). If you do, you can simply use <command>svn ci</command>. > If you do > +not, send the patch to the owner of the module for integration, > with a brief > +description of your changes for the source control logs: > +</para> > + > +<sect3 id="kernel-ci"> > +<title>IMP kernel</title> > +<para> > +All changes to the IMP kernel, since they affect everybody, must be > approved > +by Ben. Send your patch to <email>imp-dev(a)salilab.org</email> for > integration. > +</para> > +</sect3> > + > +<sect3 id="core-misc-ci"> > +<title>core and misc modules</title> > +<para> > +Anybody can commit to these modules after approval; ask Ben to get > your > +account activated. > +</para> > +</sect3> > + > +<sect3 id="other-ci"> > +<title>Other modules</title> > +<para> > +Domain-specific modules all have a responsible owner, and it is up > to them > +whether they want to allow others commit rights. Email them and ask > for > +commit rights (a sysadmin can add these), or send them your patch for > +integration. If in doubt, post it on the <email>imp- > dev(a)salilab.org</email> > +mailing list. > +</para> > +</sect3> > + > +</sect2> > + > +<sect2 id="code-maturity"> > +<title>Code maturity</title> > +<para> > +IMP is a work shared by many developers, and they would prefer not > to have > +spend time working around problems introduced by others elsewhere > in the code. > +Therefore, it is expected that any code committed to IMP should at > least > +compile. Any test cases you write for your code should also be > expected to > +pass, or at least if they do not, you should quickly fix things so > that they > +do. (As a rule of thumb, your changes should not break the nightly > builds > +for more than 3 consecutive days; if they do, your changes risk > being reverted.) > +</para> > + > +<para> > +It is fine to add code to IMP that is not yet fully complete or > ready for > +others to use (provided that it compiles and tests, as above). > However, any > +such classes or methods should be marked in their Doxygen comments as > +immature. > +</para> > + > +</sect2> > + > +<sect2 id="bugs"> > +<title>Finding and fixing bugs in IMP</title> > +<para> > +If you find a bug in IMP, please report it on the > +<ulink url="https://salilab.org/imp/bugs/">IMP bug tracker</ulink>. > This will > +ensure it does not get lost. The best way to report a bug is to > provide a > +short script file that demonstrates the problem. > +</para> > + > +<para> > +If you create a patch which fixes a bug, please cite the bug ID in > the commit > +log for that patch. The patch should also include a testcase which > triggers > +the bug, so that we can ensure your fix for the bug does not break > in future. > +In many cases, the demonstration script in the bug report can be > trivially > +adapted into a test case. > +</para> > + > +</sect2> > + > </sect1> > > </chapter> > > _______________________________________________ > IMP-commits mailing list > IMP-commits(a)salilab.org > https://salilab.org/mailman/listinfo/imp-commits

2 2

Abstracting storage
by Daniel Russel 16 Oct '08

16 Oct '08

For a while I have been thinking it would be nice to add another layer of indirection for particle storage in IMP. The idea would be that we add some base classes like: class ParticleContainer { //provide virtual get_number, get, find etc. } class ParticlePairContainer; class ParticleSetContainer; Then restraints would internally have a appropriate particle container. The nice thing would be that you could then choose to share containers between restraints and states as needed, reducing the number of things that need to be updated when something changes. This could be implemented without changing the existing API since we could leave the exiting get/set methods in the restraints. And just add a method to set the container if you want it shared. NonbondedListRestraint would go away we we would just have PairListRestraint (you just set the ParticlePairContainer in the restraint to be the NonbondedList). Likewise, the BondedList base class goes away and the BondDecoratorListRestraint. We can have containers that are collections of containers so you really only have to update things in one place. We could get rid of the problematic get_particles sorts of methods that we add for python since we can always just return this container (and I am sure we can figure out how to make it iterable). We could probably do something clever with the get_interacting_particles method so that, in most cases, it just returns the Restraints ParticleContainer instance. Would anyone else find this extra level of indirection useful?

2 2

Proposed patches to IMP
by Daniel Russel 16 Oct '08

16 Oct '08

To try to get things moving a bit, here are the first few changes I plan on submitting for people to make high level comments: - adding the introduction.dox file - adding the XYZRDecorator that has been mentioned numerous times (it now supports setting the radius attribute name) - Simplify connectivity restraint to only support sets of size 1. You can using ParticleRefiners if you want larger sets (as in Frank's paper) - add io support for the model. It is in a header called core/ model_io.h. In contrast to how it was in the introduction, the methods are just called write/read rather than write_yaml as the yaml bit is an implementation detail that there is no reason to expose and we may want to change the format at some point slightly later - change the nonbonded list to use the XYZRDecorator radius by default - add an is_nan function (this probably should go in kernel) so we don't have the funny looking x != x in places and the intent of the code is easier to understand. - rearrange the BondedLists. I think the best idea there is to replace BondedListScoreState with a simple ParticlePairsContainer (since that is all it is) and provide a few implementations of that one of which is equivalent to the BondDecoratorListScoreState.

1 0

macro names
by Daniel Russel 08 Oct '08

08 Oct '08

We aren't very consistent about macro names throughout IMP. I would suggest IMP_CORE_BEGIN_NAMESPACE IMP_CORE_EXPORT IMP_EXPORT in addition, the __ in the header guard names doesn't add anything and is technically illegal in C++/C.

2 6

Standard macros
by Daniel Russel 08 Oct '08

08 Oct '08

I would suggest having one header in each module called basic.h or base_types.h or something similar which does the job of base_types.h in IMP and defines the export and namespace macros. Currently "core_exports.h" does this, but it took me a bit to find that for the namespace macros as it isn't export related.

2 3

Minor issues encountered
by Daniel Russel 08 Oct '08

08 Oct '08

domino test should depend on core pyext, but it currently does not. The error messages for test failures have the file paths based out of the wrong directory (modules/module/test rather than the directory that scons test was run from). This breaks emacs and other programs which attempt to locate the files which caused the error. I typically run the output through sed (or use a modified run-all-tests.py which works from the root directory) to get around this, but that is a bit annoying and it would be really nice if the all paths were relative to the root dir.

2 2

New 'core' module
by Ben Webb 07 Oct '08

07 Oct '08

If you plan on using or updating IMP, please note that as of r760 most of the concrete functionality of IMP (e.g. restraints, scoring functions, decorators) has moved from the kernel to a new 'core' module, as previously discussed. If you do update IMP, be sure to run 'scons test' to make sure everything works correctly. You will also need to change any Python scripts to use the core module - e.g. rather than import IMP ... r = IMP.DistanceRestraint(...) it is now import IMP.core ... r = IMP.core.DistanceRestraint(...) For C++ code, use IMP::core::DistanceRestraint rather than IMP::DistanceRestraint, #include <IMP/core/DistanceRestraint.h> rather than <IMP/restraints/DistanceRestraint.h>, and link against libimp_core in addition to the kernel in libimp. Let me know if you run into any problems. Once the new code has worked its way through the build system and has passed all of its tests, and I have policies in the manual, I'll open up the repository for write access. 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