December 2007 - IMP-dev

Removing names from restraints
by Daniel Russel 21 Dec '07

21 Dec '07

API changes: Restraint.set_name/Restraint.get_name went away because no one was using them. There are a couple of minor changes: - RestraintSet uses the IMP_CONTAINER macros. - RestraintSet uses an auto_ptr instead of catching all exceptions Index: test/xml/test_xml.py =================================================================== --- test/xml/test_xml.py (revision 664) +++ test/xml/test_xml.py (working copy) @@ -35,18 +35,6 @@ #restraint_sets = self.imp_model.get_restraints self.assertEqual(self.imp_model.number_of_restraints(), 3, "xml file contains three restraint sets") - self.assertEqual(self.imp_model.get_restraint(IMP.RestraintIndex(0)).get_name(), - 'exclusion_volumes', - "not expecting restraint set name: " - + self.imp_model.get_restraint(IMP.RestraintIndex(0)).get_name()) - self.assertEqual(self.imp_model.get_restraint(IMP.RestraintIndex(1)).get_name(), - 'torus', - "not expecting restraint set name: " - + self.imp_model.get_restraint(IMP.RestraintIndex(1)).get_name()) - self.assertEqual(self.imp_model.get_restraint(IMP.RestraintIndex(2)).get_name(), - 'connections', - "not expecting restraint set name: " - + self.imp_model.get_restraint(IMP.RestraintIndex(2)).get_name()) # test restraints score = self.imp_model.get_restraint(IMP.RestraintIndex(0)).evaluate(None) Index: include/IMP/utility.h =================================================================== --- include/IMP/utility.h (revision 664) +++ include/IMP/utility.h (working copy) @@ -164,7 +164,7 @@ \param[in] init Code to modify the passed in object. The object is obj its index index. */ -#define IMP_CONTAINER_IMPL(Class, Ucname, lcname, IndexType, init) \ +#define IMP_CONTAINER_IMPL(Class, Ucname, lcname, IndexType, init) \ IndexType Class::add_##lcname(Ucname *obj) { \ IndexType index(lcname##_vector_.size()); \ lcname##_vector_.push_back(obj); \ Index: include/IMP/restraints/RestraintSet.h =================================================================== --- include/IMP/restraints/RestraintSet.h (revision 664) +++ include/IMP/restraints/RestraintSet.h (working copy) @@ -25,32 +25,20 @@ IMP_RESTRAINT("0.5", "Daniel Russel") - //! The type to use to retrieve a restraint - typedef int RestraintIndex; + IMP_CONTAINER(Restraint, restraint, RestraintIndex); + public: + //! Set weight for all restraints contained by this set. - /** \param[in] weight The new value of the weight. + /** Setting the weight to 0 disables the restraints in the set. + + \param[in] weight The new value of the weight. */ void set_weight(Float weight) { weight_ = weight; } - //! Add restraint to the restraint set. - /** \param[in] restraint The restraint to add to the restraint set. - \return the index of the newly-added restraint in the restraint set. - */ - RestraintIndex add_restraint(Restraint *restraint); + //! Get weight for all restraints contained by this set. + Float get_weight() { return weight_; } - //! Access a restraint in the restraint set. - /** \param[in] i The RestraintIndex of the restraint to retrieve. - \exception std::out_of_range the index is out of range. - \return Pointer to the Restraint. - */ - Restraint *get_restraint(RestraintIndex i) const; - - //! Return the total number of restraints - unsigned int number_of_restraints() const { - return restraints_.size(); - } - //! Called when at least one particle has been inactivated. /** Check each restraint to see if it changes its active status. */ @@ -58,15 +46,10 @@ protected: - //! Restraints to evaluate. - /** These can be accessed with an iterator by a filter. - In the case where the restraint is a single simple restraint, - this vector contains a pointer to itself. - */ - std::vector<Restraint *> restraints_; - //! Weight for all restraints. Float weight_; + + std::string name_; }; } // namespace IMP Index: include/IMP/Restraint.h =================================================================== --- include/IMP/Restraint.h (revision 664) +++ include/IMP/Restraint.h (working copy) @@ -29,8 +29,8 @@ class IMPDLLEXPORT Restraint : public Object { public: - //! Initialize the Restraint and its model pointer - Restraint(std::string name=std::string()); + //! Initialize the Restraint + Restraint(); virtual ~Restraint(); //! Return the score for this restraint for the current state of the model. @@ -65,16 +65,6 @@ virtual std::string last_modified_by() const = 0; - //! Get the name of the restraint - const std::string& get_name() const { - return name_; - } - - //! Set the name of the restraint - void set_name(const std::string &name) { - name_=name; - } - //! The model the restraint is part of. /** \param[in] model The model. */ @@ -134,8 +124,6 @@ bool are_particles_active_; std::vector<Particle*> particles_; - - std::string name_; }; IMP_OUTPUT_OPERATOR(Restraint); Index: impEM/pyext/IMPEM.i =================================================================== --- impEM/pyext/IMPEM.i (revision 664) +++ impEM/pyext/IMPEM.i (working copy) @@ -28,6 +28,7 @@ } /* Get definitions of IMP base classes (but do not wrap; that is done by IMP) */ +%import "IMP/Object.h" %import "IMP/Restraint.h" /* Get definitions of EMLIB base classes (but do not wrap) */ Index: src/Model.cpp =================================================================== --- src/Model.cpp (revision 664) +++ src/Model.cpp (working copy) @@ -105,7 +105,7 @@ for (RestraintIterator it = restraints_begin(); it != restraints_end(); ++it) { IMP_CHECK_OBJECT(*it); - IMP_LOG(VERBOSE, (*it)->get_name() << ": " << std::flush); + IMP_LOG(VERBOSE, **it); Float tscore=0; if ((*it)->get_is_active()) { tscore = (*it)->evaluate(accpt); @@ -203,12 +203,6 @@ out << "version: " << version() << " "; out << "last_modified_by: " << last_modified_by() << std::endl; out << number_of_particles() << " particles" << std::endl; - out << "Restraints:" << std::endl; - for (RestraintConstIterator it = restraints_begin(); - it != restraints_end(); ++it) { - IMP_CHECK_OBJECT(*it); - out << (*it)->get_name() << std::endl; - } internal::show_attributes(out); } Index: src/Restraint.cpp =================================================================== --- src/Restraint.cpp (revision 664) +++ src/Restraint.cpp (working copy) @@ -15,7 +15,7 @@ namespace IMP { -Restraint::Restraint(std::string name): name_(name) +Restraint::Restraint() { model_ = NULL; is_active_ = true; // active by default @@ -57,8 +57,8 @@ out << "unknown restraint (inactive):" << std::endl; } - out << "version: " << version() << std::endl; - out << "last_modified_by: " << last_modified_by() << std::endl; + out << " version: " << version() << std::endl; + out << " last_modified_by: " << last_modified_by() << std::endl; } Index: src/restraints/RestraintSet.cpp =================================================================== --- src/restraints/RestraintSet.cpp (revision 664) +++ src/restraints/RestraintSet.cpp (working copy) @@ -10,6 +10,7 @@ #include "IMP/log.h" #include "IMP/restraints/RestraintSet.h" #include "../mystdexcept.h" +#include <utility> namespace IMP { @@ -17,12 +18,14 @@ //! Constructor RestraintSet::RestraintSet(const std::string& name) - : Restraint(name), weight_(1.0) + : weight_(1.0) { IMP_LOG(VERBOSE, "Restraint set constructed"); + name_=name; } + //! Destructor /** \todo Should reference count restraints correctly, to avoid deleting restraints here which live in two or more RestraintSets. @@ -30,39 +33,15 @@ RestraintSet::~RestraintSet() { IMP_LOG(VERBOSE, "Delete RestraintSet"); - for (unsigned int i = 0; i < restraints_.size(); ++i) { - restraints_[i]->set_model(NULL); - delete restraints_[i]; - } + IMP_CONTAINER_DELETE(Restraint, restraint); } +IMP_CONTAINER_IMPL(RestraintSet, Restraint, restraint, RestraintIndex, + obj->set_model(get_model());); -//! Add restraint to the restraint set. -/** \param[in] restraint The restraint to add to the restraint set. - \return the index of the newly-added restraint in the restraint set. - */ -RestraintSet::RestraintIndex RestraintSet::add_restraint(Restraint* restraint) -{ - restraints_.push_back(restraint); - restraint->set_model(get_model()); - return restraints_.size() - 1; -} -//! Access a restraint in the restraint set. -/** \param[in] i The RestraintIndex of the restraint to retrieve. - \exception std::out_of_range the index is out of range. - \return Pointer to the Restraint. - */ -Restraint* RestraintSet::get_restraint(RestraintIndex i) const -{ - IMP_check(static_cast<unsigned int>(i) < restraints_.size(), - "Invalid restraint requested", - std::out_of_range("Invalid restraint")); - return restraints_[i]; -} - //! Calculate the score for this restraint for the current model state. /** \param[in] accum If not NULL, use this object to accumulate partial first derivatives. @@ -70,25 +49,21 @@ */ Float RestraintSet::evaluate(DerivativeAccumulator *accum) { + if (get_weight() == 0) return 0; Float score; - + typedef std::auto_ptr<DerivativeAccumulator> DAP; // Use a local copy of the accumulator for our sub-restraints - DerivativeAccumulator *ouracc = NULL; + DAP ouracc; if (accum) { - ouracc = new DerivativeAccumulator(*accum, weight_); + ouracc = DAP(new DerivativeAccumulator(*accum, weight_)); } score = (Float) 0.0; - try { - for (size_t i = 0; i < restraints_.size(); i++) { - if (restraints_[i]->get_is_active()) - score += restraints_[i]->evaluate(ouracc); + for (RestraintIterator it= restraints_begin(); it != restraints_end(); ++it) { + if ((*it)->get_is_active()) { + score += (*it)->evaluate(ouracc.get()); } - } catch (...) { - delete ouracc; - throw; } - delete ouracc; return score * weight_; } @@ -99,8 +74,8 @@ */ void RestraintSet::check_particles_active() { - for (size_t i = 0; i < restraints_.size(); i++) { - restraints_[i]->check_particles_active(); + for (RestraintIterator it= restraints_begin(); it != restraints_end(); ++it) { + (*it)->check_particles_active(); } } @@ -111,11 +86,12 @@ */ void RestraintSet::show(std::ostream& out) const { - out << "restraint set " << get_name() << ":" << std::endl; - for (size_t i = 0; i < restraints_.size(); i++) { - restraints_[i]->show(out); + out << "restraint set " << name_ << ":..." << std::endl; + for (RestraintConstIterator it= restraints_begin(); + it != restraints_end(); ++it) { + (*it)->show(out); } - out << "... end restraint set " << get_name() << std::endl; + out << "... end restraint set " << name_ << std::endl; }

1 0

Several small patches
by Daniel Russel 20 Dec '07

20 Dec '07

First I added a pair of functions internal::assert_failure and internal::check_failure which get called when assertions and checks fail. You can have the debugger break in these to find failures. The derivative accumulator checks for NaN. And finally ConjugateGradients wasn't initializing its threshold_ member variable.

1 0

Restraint niggles
by Daniel Russel 20 Dec '07

20 Dec '07

No one seems to ever use the name on Restraints as opposed to restraint sets. I propose we get rid of the name field to make restraint lighter. I also propose we get rid of the active flag as it is not really used and can also be pushed into a restraint set if the functionality is desired. Alternatively, if we like names, it should be made non-optional.

2 1

Proposal: new OptimizerState class
by Ben Webb 20 Dec '07

20 Dec '07

We currently have a State class, which you can use to update some kind of shared state every time the score is calculated. You could also in principle use State to generate a trajectory during an optimization (e.g. see test/md_optimizer/test_md_optimizer.py). This works fine for optimizers such as molecular dynamics, where each step of the optimization has a single evaluation of the score function. But it's no good for optimizers like CG and SD, which may do multiple trial evaluations per step - you only want to include the accepted points in the trajectory. You could also imagine doing other things during an optimization - for example, periodic recentering of the system, resetting of thermostats, or rescaling of particle velocities. Thus, I propose an OptimizerState class, which is similar to State but is added to an optimizer rather than a model. Its update method would then be called at every successful optimization step, rather than at every score evaluation. To prevent people adding OptimizerStates to the model, the current State should be renamed ScoreState and both OptimizerState and ScoreState should derive from a common State base class. A derived MDOptimizerState or similar class could be used in turn as a base class for MD-specific stuff such as velocity scaling. 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 2

Proposed bonded/nonbonded list interfaces
by Daniel Russel 20 Dec '07

20 Dec '07

Since I needed caching of my bonds so I decided to refactor things to have bonded and nonbonded lists. Here are my proposed interfaces for people's perusal. I then have a DynamicExclusionVolumeRestraint and DynamicBondRestraint which take pointers to these states and get their particle pairs from them. I don't know the best way to make things accessible from python (I don't think the C++ iterators will work from python, but I haven't tried). For reference, a BondDecorator is a decorator wrapping a particle which contains information (length, stiffness, type) for a bond. //! A pair of Particle * which can be used in maps struct ParticlePair { ParticlePair(Particle *a, Particle *b); Particle *get_particle(unsigned int i) const ; IMP_COMPARISONS_2(p_[0], p_[1]); }; //! A state that maintains a list of bonded particles. /** The state uses the BondedDecorator to find bonded pairs in its list of particles and builds a fast lookup structure on them. */ class IMPDLLEXPORT BondedListState : public State { public: BondedListState(const Particles &pis); virtual ~BondedListState(); IMP_STATE("0.5", "Daniel Russel"); void set_particles(const Particles &pis) ; typedef std::map<ParticlePair, BondDecorator>::const_iterator BondsIterator; BondsIterator bonds_begin() const ; BondsIterator bonds_end() const ; //! Get a bonds between two particles /** \returns BondDecorator() if there is not bond or the appropriate one if there is a bond. */ BondDecorator get_bond(Particle *a, Particle *b) const ; }; //! A state that maintains a list of nonbonded particles. /** */ class IMPDLLEXPORT NonbondedListState : public State { public: /** \note The distance_threshold parameter is just a guideline. Particle pairs with larger distance can be returned. But all with smaller distance will be. BondedListState will be updated when this one is and deleted when this State is and so should not be added seperately to the Model. */ NonbondedListState(const Particles &pis, Float distance_threshold, BondedList *bl=NULL); virtual ~NonbondedListState(); IMP_STATE("0.5", "Daniel Russel"); void set_particles(const Particles &pis); /*Eventually these iterators could generate the pairs on the fly from the voxel grid*/ typedef std::vector<ParticlePair>::const_iterator NonbondsIterator; NonbondsIterator nonbonds_begin() const ; NonbondsIterator nonbonds_end() const ; };

2 4

Re: [IMP-dev] various evils
by Ben Webb 18 Dec '07

18 Dec '07

Daniel Russel wrote: > I think it is a bit evil to have NDEBUG defined when building the swig > library but not any other time. Any function declared in a header can > then asserts turned off. > > Even worse, various things can change memory layout when NDEBUG is > defined, resulting in random crashes if modules are mixed. We probably > should make sure IMP doesn't have this property, but I have encountered > other libraries which do such things. I think 'evil' is a strong word for it. ;) But as of r654, there is a 'release' scons option. By default it is False, which will build both regular libraries and SWIG extensions without -DNDEBUG. If you turn release on, it'll set NDEBUG for everything. But we definitely shouldn't have different memory layouts in the two cases - that is a really sloppy ABI. > On a separate note, since it has been bugging me a bit, you are not > supposed to use identifiers starting with _ or __ as these are reserved > for the compiler. All our header guards start with __. Frankly, I don't > expect this to ever cause problems, but such are the rules. A huge number of libraries use __ for their header guards, so I'd be very surprised if this were to ever cause problems. For example, freetype, gmp, mpfr and glib on my Mac use __. A bunch of others use a single _, and those with _ or __ are a distinct minority (the situation is even more pronounced for the stuff in /usr/include). So I think you can sleep comfortably. 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

Suggested IMP cleanups
by Daniel Russel 18 Dec '07

18 Dec '07

I would like to propose getting rid of the trajectory log in the model as we can do the same sort of thing using State classes. Since these aren't part of the Model, they would be much more flexible and customizable. I currently have such a state for writing chimera BILD files that I can submit at some point. And I would propose removing the ModelData OptimizedIndexIterator (or whatever it is called). The API is funny and it doesn't provide any functionality which can't be achieved other ways using more or less the same number of lines of code.

2 2

Annoyance with current restraint design
by Daniel Russel 18 Dec '07

18 Dec '07

I have some restraints which I want to have be semi-dynamic, meaning, I want to be able to reset the list of particles which they act on. This doesn't work so cleanly with restraints taking lists of ParticleIndexes as then you need one version of set_particles to be called from the constructor (where the Restraint::model_ is not set) and another version to be called by outsiders after the model pointer has been set (or pass the model pointer to both and ignore the stored pointer). This seems like a reason for writing restraints which take Particles rather than ParticleIndexes. It may also be worth dropping the Restraint::model_ pointer entirely and just using the pointer stored in particles. So DistanceRestraint::DistanceRestraint(Particle*, Particle*, ScoreFunc*) ExclusionVolumeRestraint(Particles, ScoreFuncParams) Any thoughts?

2 2

Removing superfluous Model*
by Daniel Russel 18 Dec '07

18 Dec '07

As mentioned yesterday I removed the superfluous Model * from DistanceRestraint and SphericalRestraint. API changes: DistanceRestaint::DistanceRestraint and SphericalRestraint::SphericalRestraint no longer take a Model* as the first argument.

1 0

Common object base
by Daniel Russel 18 Dec '07

18 Dec '07

I was worried that I was having issues with python prematurely deleting objects so I added a common base class on the heap allocated objects (to replace boost::noncopyable). This class enforces the non- copyability as well as uses a conservative heuristic to keep track of when the object has been deleted but is still in use and to try to throw an exception when such has been detected. We don't currently have issues with object ownership in IMP, but they are a pain to track down when they occur as python generally just unceremoniously bombs. And it gets rid of the funny IMP/boost files. A few other bounds checks and null pointer checks have been added also. There are no API changes associated with this patch.

2 1