NBL cleanup and other small patches
Frido was complaining that the nbl was too slow, so I thought I would check in my local changes. The big thing is that the fastest implementation I have uses the CGAL library. It is part of fedora extras so getting it is not that big a deal (and I think there are installers for macs). The nbl code is controlled by the macro IMP_USE_CGAL so everything works fine without it. I have a patched scons init file which adds support (but does not do a very good job since I don't know how to write link tests and such).
First some small patches: The first adds a constructor to decorators which assumes that the particles have the needed fields (this is checked if debug code is used). We should add checks to the python interface eventually.
The second patches the init file as above. It also adds an argument extraroot which adds to the include search path and the lib path (and to the runtime link path since it is needed to make the boost tests pass currently). There are probably lots of ways to make this better as I don't really know scons. Also, as a general point, the SConscript files which build things should all be changed to add the local include and lib paths to the front so that local headers mask globally installed headers.
The third prints a nicer error message if you forget to set the model before CG::optimize (this is independent of the other patches)
The fourth uses the decorator constructors in the bond decorators.
The fifth adds set_particles to the the list restraint I added before (this is independent of anything else).
And the fifth patches the nonbonded lists. There is now just AllNonbondedListScoreState and BipartiteNonbondedListScore state with set_algorithm methods (and arguments to the constructor) to determine which algorithm is used. The default is either grid or CGAL if the latter is available. The other interface change is that the nbl now takes the distance cutoff in its constructor (rather than the cutoff being passed to the NBRestraint as before). This simplified things a great deal. The patch also brings in a BondedList for chains which should be faster than using the bond decorators.
Some timing results for the various nbls follow. The results are normalized by the number of particles involved. Nothing: (just run the python code to set up the particles) For 50 particles got 3.24961185455 For 100 particles got 3.17380809784 For 1000 particles got 3.14385008812 For 10000 particles got 3.13716006279 For 100000 particles got 3.16522884369 Bbox- (CGAL) For 50 particles got 3.32897806168 For 100 particles got 3.29577898979 For 1000 particles got 3.42112493515 For 10000 particles got 4.14875411987 For 100000 particles got 5.81269311905 Grid For 50 particles got 3.63301706314 For 100 particles got 3.84657287598 For 1000 particles got 6.80274581909 For 10000 particles got 11.4696931839 Quadratic For 50 particles got 3.39585018158 For 100 particles got 3.51785087585 For 1000 particles got 6.79182696342 For 10000 particles got 44.5892150402
Index: kernel/include/IMP/decorators/macros.h =================================================================== --- kernel/include/IMP/decorators/macros.h (revision 524) +++ kernel/include/IMP/decorators/macros.h (working copy) @@ -44,15 +44,17 @@ add_required; \ } \ friend class DecoratorBase; \ - Name(::IMP::Particle* p): Parent(p) { \ - IMP_assert(has_required_attributes(p), \ - "This is not a particle of type " \ - << #Name << *p); \ - } \ public: \ typedef Name This; \ - /** The default constructor. This is used as a null value */ \ + /** \short The default constructor. This is used as a null value */ \ Name(): Parent(){} \ + /** \short Construct from a Particle which has all needed attributes */\ + Name(::IMP::Particle *p): Parent(p){ \ + if (!decorator_keys_initialized_) decorator_initialize_static_data();\ + IMP_assert(has_required_attributes(p), \ + "Particle missing required attributes for decorator" \ + << #Name << *p << std::endl); \ + } \ /** Add the necessary attributes to p and return a decorator. */ \ static Name create(::IMP::Particle *p) { \ return IMP::DecoratorBase::create<Name>(p); \
Index: tools/__init__.py =================================================================== --- tools/__init__.py (revision 524) +++ tools/__init__.py (working copy) @@ -53,7 +53,10 @@ def _add_release_flags(env): """Add compiler flags for release builds, if requested""" if env.get('release', False): - env.Append(CPPDEFINES='NDEBUG') + env.Append(CPPDEFINES=['NDEBUG']) + env.Append(CCFLAGS=["-O3"]) + else: + env.Append(CCFLAGS=["-g"])
def CheckGNUHash(context): """Disable GNU_HASH-style linking (if found) for backwards compatibility""" @@ -186,16 +189,30 @@ pass env.Prepend(SCANNERS = _SWIGScanner) if env['CC'] == 'gcc': - env.Append(CCFLAGS="-Wall -g -O3") + env.Append(CCFLAGS=["-Wall"]) _add_release_flags(env)
sys = platform.system() if sys == 'SunOS': # Find locally-installed libraries in /usr/local (e.g. for SWIG) env['ENV']['LD_LIBRARY_PATH'] = '/usr/local/lib' - # Make Modeller exetype variable available: - if os.environ.has_key('EXECUTABLE_TYPESVN'): - env['ENV']['EXECUTABLE_TYPESVN'] = os.environ['EXECUTABLE_TYPESVN'] + + if (ARGUMENTS.get('extraroot', '') != ''): + env.Append(CPPPATH=['$extraroot/include']) + env.Append(LIBPATH=['$extraroot/lib']) + env.Append(LINKFLAGS=['-Wl,-rpath,$extraroot/lib']) + env.Append(LD_LIBRARY_PATH=['$extraroot/lib']) + env['EXTRA_LDPATH']=ARGUMENTS.get('extraroot', '')+'/lib' + env['EXTRA_PYTHONPATH']=ARGUMENTS.get('extraroot', '')+'/lib/python2.5/site-packages' + + + if (ARGUMENTS.get('usecgal')): + # should do tests here to make sure it is usable, but I don't know how + env.Append(CPPDEFINES=['IMP_USE_CGAL']) + env.Append(LIBS= ['CGAL']) + if env['CC'] == 'gcc': + env.Append(CCFLAGS=["-frounding-math"]) + # Set empty variables in case the Modeller check fails: for mod in ('MODPY', 'EXETYPE'): env['MODELLER_' + mod] = '' @@ -309,6 +326,8 @@ # Remove options that don't work with C++ code: if cplusplus: opt = opt.replace("-Wstrict-prototypes", "") + if cplusplus: + opt = opt.replace("-O2", "") e.Replace(CC=cc, CXX=cxx, LDMODULESUFFIX=so) e.Replace(CPPFLAGS=basecflags.split() + opt.split())
@@ -372,3 +391,10 @@ opts.Add(BoolOption('release', 'Disable most runtime checks (e.g. for releases)', False)) + opts.Add(PathOption('extraroot', + 'Extra place to search for includes and libs', + '', PathOption.PathAccept)) + # I would like to have a check, but it doesn't seem to turn it off it no value is passed + #PathOption.PathIsDir)) + opts.Add(BoolOption('usecgal', + 'Use the CGAL library for some algorithms', False))
Index: kernel/src/optimizers/ConjugateGradients.cpp =================================================================== --- kernel/src/optimizers/ConjugateGradients.cpp (revision 524) +++ kernel/src/optimizers/ConjugateGradients.cpp (working copy) @@ -214,6 +214,9 @@
Float ConjugateGradients::optimize(unsigned int max_steps) { + IMP_check(get_model(), + "Must set the model on the optimizer before optimizing", + ValueException("Must set the model before optimizing")); std::vector<Float> x, dx; int i; //ModelData* model_data = get_model()->get_model_data();
Index: kernel/src/decorators/bond_decorators.cpp =================================================================== --- kernel/src/decorators/bond_decorators.cpp (revision 524) +++ kernel/src/decorators/bond_decorators.cpp (working copy) @@ -77,7 +77,7 @@ { Particle *p= internal::graph_connect(a.get_particle(), b.get_particle(), internal::bond_graph_data_); - BondDecorator bd= BondDecorator::cast(p); + BondDecorator bd(p); bd.set_type(t); return bd; } Index: kernel/include/IMP/decorators/bond_decorators.h =================================================================== --- kernel/include/IMP/decorators/bond_decorators.h (revision 524) +++ kernel/include/IMP/decorators/bond_decorators.h (working copy) @@ -102,7 +102,7 @@ BondDecorator get_bond(unsigned int i) const { Particle *p= graph_get_edge(get_particle(), i, internal::bond_graph_data_); - return BondDecorator::cast(p); + return BondDecorator(p); }
//! Get a BondedDecorator of the ith bonded particle @@ -117,7 +117,7 @@ */ BondedDecorator get_bonded(unsigned int i) const { Particle *p= graph_get_edge(get_particle(), i, internal::bond_graph_data_); - BondDecorator bd= BondDecorator::cast(p); + BondDecorator bd(p); if (bd.get_bonded(0) == *this) return bd.get_bonded(1); else return bd.get_bonded(0); } @@ -130,7 +130,7 @@ { Particle *p= graph_get_node(get_particle(), i, internal::bond_graph_data_); - return BondedDecorator::cast(p); + return BondedDecorator(p); }
//! Connect the two wrapped particles by a bond.
Index: kernel/include/IMP/restraints/SingletonListRestraint.h =================================================================== --- kernel/include/IMP/restraints/SingletonListRestraint.h (revision 524) +++ kernel/include/IMP/restraints/SingletonListRestraint.h (working copy) @@ -38,6 +38,7 @@
using Restraint::add_particles; using Restraint::clear_particles; + using Restraint::set_particles;
protected: internal::ObjectPointer<SingletonScore, true> ss_;
Index: kernel/include/IMP/restraints/NonbondedRestraint.h =================================================================== --- kernel/include/IMP/restraints/NonbondedRestraint.h (revision 524) +++ kernel/include/IMP/restraints/NonbondedRestraint.h (working copy) @@ -34,10 +34,8 @@ object is deleted upon destruction. \param[in] nbl The non-bonded list to use to get the list of particles. - \param[in] max_dist Pairs beyond this distance may be dropped. */ - NonbondedRestraint(PairScore *ps, NonbondedListScoreState *nbl, - Float max_dist= std::numeric_limits<Float>::max()); + NonbondedRestraint(PairScore *ps, NonbondedListScoreState *nbl); virtual ~NonbondedRestraint(){}
IMP_RESTRAINT(internal::kernel_version_info) @@ -45,7 +43,6 @@ protected: NonbondedListScoreState *nbl_; internal::ObjectPointer<PairScore, true> sf_; - Float max_dist_; };
} // namespace IMP Index: kernel/src/restraints/NonbondedRestraint.cpp =================================================================== --- kernel/src/restraints/NonbondedRestraint.cpp (revision 524) +++ kernel/src/restraints/NonbondedRestraint.cpp (working copy) @@ -17,9 +17,8 @@ {
NonbondedRestraint::NonbondedRestraint(PairScore *ps, - NonbondedListScoreState *nbl, - Float md) : nbl_(nbl), sf_(ps), - max_dist_(md) + NonbondedListScoreState *nbl) + : nbl_(nbl), sf_(ps) { }
@@ -31,12 +30,12 @@ IMP_CHECK_OBJECT(nbl_); Float score=0; IMP_LOG(VERBOSE, "Nonbonded restraint on " - << std::distance(nbl_->nonbonded_begin(max_dist_), - nbl_->nonbonded_end(max_dist_)) + << std::distance(nbl_->nonbonded_begin(), + nbl_->nonbonded_end()) << " pairs" << std::endl); for (NonbondedListScoreState::NonbondedIterator it - = nbl_->nonbonded_begin(max_dist_); - it != nbl_->nonbonded_end(max_dist_); ++it) { + = nbl_->nonbonded_begin(); + it != nbl_->nonbonded_end(); ++it) { float thisscore = sf_->evaluate(it->first, it->second, accum); if (thisscore != 0) { IMP_LOG(VERBOSE, "Pair " << it->first->get_index() Index: kernel/include/IMP/score_states/QuadraticNonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/QuadraticNonbondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/QuadraticNonbondedListScoreState.h (working copy) @@ -1,51 +0,0 @@ -/** - * \file QuadraticNonbondedListScoreState.h - * \brief Allow iteration through pairs of a set of spheres. - * - * Copyright 2007-8 Sali Lab. All rights reserved. - */ - -#ifndef __IMP_QUADRATIC_NONBONDED_LIST_SCORE_STATE_H -#define __IMP_QUADRATIC_NONBONDED_LIST_SCORE_STATE_H - -#include "NonbondedListScoreState.h" -#include "../internal/kernel_version_info.h" -#include "MaxChangeScoreState.h" - -#include <vector> -#include <limits> - -namespace IMP -{ - -//! A base class for nonbonded lists which test all pairs of particles -/** This should not be used by end users. But since it needs to be in the - inheritance hierarchy, it should be in the IMP namespace. - */ -class IMPDLLEXPORT QuadraticNonbondedListScoreState: - public NonbondedListScoreState -{ - typedef NonbondedListScoreState P; - internal::ObjectPointer<MaxChangeScoreState, true> mc_; - float slack_; - protected: - void handle_nbl_pair( Particle *a, Particle *b, float d); - const Particles &get_particles() const { - return mc_->get_particles(); - } - void set_particles(const Particles &ps) { - P::clear_nbl(); - mc_->clear_particles(); - mc_->add_particles(ps); - } - - QuadraticNonbondedListScoreState(FloatKey radius); - ~QuadraticNonbondedListScoreState(); - - public: - void update(); -}; - -} // namespace IMP - -#endif /* __IMP_QUADRATIC_NONBONDED_LIST_SCORE_STATE_H */ Index: kernel/include/IMP/score_states/BondDecoratorListScoreState.h =================================================================== --- kernel/include/IMP/score_states/BondDecoratorListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/BondDecoratorListScoreState.h (working copy) @@ -8,10 +8,11 @@ #ifndef __IMP_BOND_DECORATOR_LIST_SCORE_STATE_H #define __IMP_BOND_DECORATOR_LIST_SCORE_STATE_H
-#include <vector> #include "BondedListScoreState.h" #include "../decorators/bond_decorators.h"
+#include <vector> + namespace IMP {
@@ -25,6 +26,7 @@ */ class IMPDLLEXPORT BondDecoratorListScoreState: public BondedListScoreState { + // This is only used to provide the iterators. Probably should be lazy. std::vector<BondDecorator> bonds_; Particles ps_; public: Index: kernel/include/IMP/score_states/NonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/NonbondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/NonbondedListScoreState.h (working copy) @@ -11,6 +11,7 @@ #include "../ScoreState.h" #include "../internal/ParticleGrid.h" #include "../internal/kernel_version_info.h" +#include "../decorators/XYZDecorator.h" #include "BondedListScoreState.h"
#include <vector> @@ -19,34 +20,52 @@ namespace IMP {
+namespace internal +{ +class NBLAddPairIfNonbonded; +class NBLAddIfNonbonded; +} + typedef std::vector<BondedListScoreState*> BondedListScoreStates;
-//! A base class for classes that maintain a list of non-bonded pairs. -/** +//! An abstract base class for classes that maintain a list of non-bonded pairs. +/** \note If no value for the radius key is specified, all radii are + considered to be zero. */ class IMPDLLEXPORT NonbondedListScoreState: public ScoreState { private: FloatKey rk_; - -protected: + // the distance cutoff to count a pair as adjacent + float cutoff_; + /* How much to add to the size of particles to allow them to move + without rebuilding*/ + Float slack_; + /* An estimate of what the slack should be next time they are recomputed*/ + Float next_slack_; + int num_steps_; + bool nbl_is_valid_; // made protected for debugging code, do not use otherwise typedef std::vector<std::pair<Particle*, Particle*> > NBL; NBL nbl_; - float last_cutoff_;
+protected: + + Float get_slack() const {return slack_;} + Float get_cutoff() const {return cutoff_;} + unsigned int size_nbl() const {return nbl_.size();}
//! rebuild the nonbonded list - /** \internal + /** This is used by classes which inherit from this to rebuild the NBL + + \internal */ - virtual void rebuild_nbl(float cut)=0; + virtual void rebuild_nbl()=0;
- void clear_nbl() { - last_cutoff_=-1; - nbl_.clear(); - }
+ + // Return true if two particles are bonded bool are_bonded(Particle *a, Particle *b) const { IMP_assert(a->get_is_active() && b->get_is_active(), "Inactive particles should have been removed" @@ -60,27 +79,13 @@ return false; }
- struct AddToNBL; - friend struct AddToNBL; + friend struct internal::NBLAddPairIfNonbonded; + friend struct internal::NBLAddIfNonbonded;
- // these should be two separate classes, but they are not - struct AddToNBL{ - NonbondedListScoreState *state_; - Particle *p_; - AddToNBL(NonbondedListScoreState *s, Particle *p): state_(s), - p_(p){} - void operator()(Particle *p) { - operator()(p_, p); - } - void operator()(Particle *a, Particle *b) { - state_->add_to_nbl(a,b); - } - };
- //! tell the bonded lists what particles to pay attention to - void propagate_particles(const Particles &ps);
- void add_to_nbl(Particle *a, Particle *b) { + // Add the pair to the nbl if the particles are not bonded + void add_if_nonbonded(Particle *a, Particle *b) { IMP_assert(a->get_is_active() && b->get_is_active(), "Inactive particles should have been stripped");
@@ -95,6 +100,28 @@ } }
+ // Check if the bounding boxes of the spheres overlap + void add_if_box_overlap(Particle *a, Particle *b) { + XYZDecorator da(a); + XYZDecorator db(b); + float ra= get_radius(a); + float rb= get_radius(b); + for (unsigned int i=0; i< 3; ++i) { + float delta=std::abs(da.get_coordinate(i) - db.get_coordinate(i)); + if (delta -ra -rb > cutoff_+slack_) { + IMP_LOG(VERBOSE, "Pair " << a->get_index() + << " and " << b->get_index() << " rejected on coordinate " + << i << " " << da.get_coordinate(i) << " and " + << db.get_coordinate(i) << " " << ra << " " + << rb << " " << slack_ << " " << slack_ << std::endl); + return ; + } + } + IMP_LOG(VERBOSE, "Adding pair " << a->get_index() + << " and " << b->get_index() << std::endl); + add_if_nonbonded(a, b); + } + Float get_radius(Particle *a) const { if (rk_ != FloatKey() && a->has_attribute(rk_)) { return a->get_value(rk_); @@ -102,11 +129,26 @@ return 0; } } + // return true if the nbl was invalidated by a move of mc + bool update(Float mc);
+ // Return a list of all the particles in in with a radius field + Particles particles_with_radius(const Particles &in) const; + + + // Return true if the nbl is valid + bool get_nbl_is_valid() const {return nbl_is_valid_;} + + void set_nbl_is_valid(bool tf) { + nbl_is_valid_= tf; + if (!nbl_is_valid_) { + nbl_.clear(); + } + } public: /** */ - NonbondedListScoreState(FloatKey rk); + NonbondedListScoreState(Float cutoff, FloatKey rk);
FloatKey get_radius_key() const {return rk_;} void set_radius_key(FloatKey rk) {rk_=rk;} @@ -118,48 +160,56 @@ typedef BondedListScoreStateIterator BondedListIterator; typedef BondedListScoreStateConstIterator BondedListConstIterator;
- IMP_SCORE_STATE(internal::kernel_version_info) + virtual void show(std::ostream &out=std::cout) const; + virtual IMP::VersionInfo get_version_info() const { + return internal::kernel_version_info; + }
//! An iterator through nonbonded particles - /** The value type is an std::pair<Particle*, Particle*> + /** The value type is an ParticlePair. */ typedef NBL::const_iterator NonbondedIterator;
//! This iterates through the pairs of non-bonded particles - /** \param[in] cutoff The state may ignore pairs which are futher - apart than the cutoff. - \note that this is highly unsafe and iteration can only be - done once at a time. I will fix that eventually. - - \note The distance cutoff is the l2 norm between the 3D coordinates - of the Particles. It ignore any size that may be associated with - the particles. - */ - NonbondedIterator nonbonded_begin(Float cutoff - =std::numeric_limits<Float>::max()) { - IMP_assert(last_cutoff_== cutoff || last_cutoff_==-1, - "Bad things are happening with the iterators in " - << "NonbondedListScoreState"); - if (last_cutoff_ < cutoff) { - IMP_LOG(VERBOSE, "Rebuilding NBL cutoff " << cutoff << std::endl); - clear_nbl(); - rebuild_nbl(cutoff); - last_cutoff_=cutoff; - } + NonbondedIterator nonbonded_begin() const { + IMP_check(get_nbl_is_valid(), "Must call update first", + ValueException("Must call update first")); return nbl_.begin(); } - NonbondedIterator nonbonded_end(Float cutoff - =std::numeric_limits<Float>::max()) { - if (last_cutoff_ < cutoff) { - IMP_LOG(VERBOSE, "Rebuilding NBL cutoff " << cutoff << std::endl); - clear_nbl(); - rebuild_nbl(cutoff); - last_cutoff_=cutoff; - } + NonbondedIterator nonbonded_end() const { + IMP_check(get_nbl_is_valid(), "Must call update first", + ValueException("Must call update first")); return nbl_.end(); } + + + unsigned int number_of_nonbonded() const { + return nbl_.size(); + } };
+ +namespace internal +{ + struct NBLAddPairIfNonbonded{ + NonbondedListScoreState *state_; + NBLAddPairIfNonbonded(NonbondedListScoreState *s): state_(s){} + void operator()(Particle *a, Particle *b) { + state_->add_if_nonbonded(a,b); + } + }; + + struct NBLAddIfNonbonded{ + NonbondedListScoreState *state_; + Particle *p_; + NBLAddIfNonbonded(NonbondedListScoreState *s, Particle *p): state_(s), + p_(p){} + void operator()(Particle *p) { + state_->add_if_nonbonded(p_, p); + } + }; +} + } // namespace IMP
#endif /* __IMP_NONBONDED_LIST_SCORE_STATE_H */ Index: kernel/include/IMP/score_states/SConscript =================================================================== --- kernel/include/IMP/score_states/SConscript (revision 524) +++ kernel/include/IMP/score_states/SConscript (working copy) @@ -1,14 +1,15 @@ +Import('env') import os.path -Import('env') - -files = ['BondedListScoreState.h', 'NonbondedListScoreState.h', - 'BipartiteNonbondedListScoreState.h', 'MaxChangeScoreState.h', - 'BondDecoratorListScoreState.h', 'AllNonbondedListScoreState.h', - 'QuadraticBipartiteNonbondedListScoreState.h', - 'QuadraticAllNonbondedListScoreState.h', - 'QuadraticNonbondedListScoreState.h', 'GravityCenterScoreState.h'] - -# Install the include files: -includedir = os.path.join(env['includedir'], 'IMP', 'score_states') +files=[ + 'AllNonbondedListScoreState.h', + 'BipartiteNonbondedListScoreState.h', + 'BondDecoratorListScoreState.h', + 'BondedListScoreState.h', + 'ChainBondedListScoreState.h', + 'GravityCenterScoreState.h', + 'MaxChangeScoreState.h', + 'NonbondedListScoreState.h', + ] +includedir = os.path.join(env['includedir'], 'IMP', 'score_states' ) inst = env.Install(includedir, files) env.Alias('install', inst) Index: kernel/include/IMP/score_states/QuadraticBipartiteNonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/QuadraticBipartiteNonbondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/QuadraticBipartiteNonbondedListScoreState.h (working copy) @@ -1,49 +0,0 @@ -/** - * \file QuadraticBipartiteNonbondedListScoreState.h - * \brief Allow iteration through pairs of a set of atoms. - * - * Copyright 2007-8 Sali Lab. All rights reserved. - */ - -#ifndef __IMP_QUADRATIC_BIPARTITE_NONBONDED_LIST_SCORE_STATE_H -#define __IMP_QUADRATIC_BIPARTITE_NONBONDED_LIST_SCORE_STATE_H - -#include "QuadraticNonbondedListScoreState.h" -#include "../internal/ParticleGrid.h" -#include "../internal/kernel_version_info.h" - -#include <vector> -#include <limits> - -namespace IMP -{ - -//! This class maintains a list of non-bonded pairs between two sets. -/** The class works roughly like the NonbondedListScoreState except - only pairs where one particle is taken from each set are returned. - - \note QuadraticBipartiteNonbondedListScoreState is basically an - implementation detail for performance analysis and should not - be used by end users. - */ -class IMPDLLEXPORT QuadraticBipartiteNonbondedListScoreState: - public QuadraticNonbondedListScoreState -{ - typedef QuadraticNonbondedListScoreState P; - unsigned int na_; - - virtual void rebuild_nbl(float cut); -public: - QuadraticBipartiteNonbondedListScoreState(FloatKey rk, - const Particles &ps0, - const Particles &ps1); - QuadraticBipartiteNonbondedListScoreState(FloatKey rk); - - IMP_SCORE_STATE(internal::kernel_version_info) - - void set_particles(const Particles &ps0, const Particles &ps1); -}; - -} // namespace IMP - -#endif /* __IMP_QUADRATIC_BIPARTITE_NONBONDED_LIST_SCORE_STATE_H */ Index: kernel/include/IMP/score_states/BipartiteNonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/BipartiteNonbondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/BipartiteNonbondedListScoreState.h (working copy) @@ -1,32 +1,92 @@ /** * \file BipartiteNonbondedListScoreState.h - * \brief Allow iteration through pairs of a set of atoms. + * \brief Bipartiteow iteration through pairs of a set of s. * - * Copyright 2007-8 Sali Lab. All rights reserved. + * Copyright 2007-8 Sali Lab. Bipartite rights reserved. */
#ifndef __IMP_BIPARTITE_NONBONDED_LIST_SCORE_STATE_H #define __IMP_BIPARTITE_NONBONDED_LIST_SCORE_STATE_H
-#include "QuadraticBipartiteNonbondedListScoreState.h" +#include "NonbondedListScoreState.h" +#include "../internal/kernel_version_info.h" +#include "MaxChangeScoreState.h"
+ namespace IMP { -//! Maintain a nonbonded list between two disjoint sets. -/** - \note If no value for the radius key is specified, all radii are - considered to be zero.
- \ingroup restraint +//! This class maintains a list of non-bonded pairs of spheres between two sets +/** To iterate through the list of pairs use the NonbondedListScoreState::begin, + NonbondedListScoreState::end functions. + + The radius key can be the default key. + + Changes in coordinates and radii are handled properly. + + \ingroup restraint */ -class BipartiteNonbondedListScoreState: - public QuadraticBipartiteNonbondedListScoreState { - typedef QuadraticBipartiteNonbondedListScoreState P; +class IMPDLLEXPORT BipartiteNonbondedListScoreState: + public NonbondedListScoreState +{ + typedef NonbondedListScoreState P; public: - BipartiteNonbondedListScoreState(FloatKey rk, + //! What algorithm to use to perform the computations + enum Algorithm { + //! Check all pairs of particles to see if they are close enough + QUADRATIC, +#ifdef IMP_USE_CGAL + //! Sweep space looking for intersecting bounding boxes. + BBOX, + DEFAULT= BBOX +#else + DEFAULT= QUADRATIC +#endif + }; +protected: + Algorithm a_; + internal::ObjectPointer<MaxChangeScoreState, true> mc0_, mc1_, mcr_; + + void process_sets(const Particles &p0, + const Particles &p1); + + //! \internal + void rebuild_nbl(); +public: + /**\param[in] cutoff The distance cutoff to use. + \param[in] radius The key to use to get the radius + \param[in] a Which algorithm to use. The default is the best. + */ + BipartiteNonbondedListScoreState(Float cutoff, FloatKey radius, + Algorithm a=DEFAULT); + /**\param[in] cutoff The distance cutoff to use. + \param[in] radius The key to use to get the radius + \param[in] ps0 The first set. + \param[in] ps1 The second set. + \param[in] a Which algorithm to use. The default is the best. + */ + BipartiteNonbondedListScoreState(Float cutoff, + FloatKey radius, const Particles &ps0, - const Particles &ps1): P(rk, ps0, ps1){} - BipartiteNonbondedListScoreState(FloatKey rk): P(rk){} + const Particles &ps1, + Algorithm a=DEFAULT); + + ~BipartiteNonbondedListScoreState(); + IMP_SCORE_STATE(internal::kernel_version_info); + + //! Add the particles to the first set + void add_particles_0(const Particles &ps); + //! Add the particles to the second set + void add_particles_1(const Particles &ps); + //! Remove all particles + void clear_particles(); + //! Replace the set of particles + void set_particles(const Particles &ps0, const Particles &ps1); + + /** If there is CGAL support, a more efficient algorithm BBOX can be used*/ + void set_algorithm(Algorithm a) { + a_=a; + } };
} // namespace IMP Index: kernel/include/IMP/score_states/BondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/BondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/BondedListScoreState.h (working copy) @@ -25,9 +25,6 @@ BondedListScoreState(){} virtual ~BondedListScoreState(){}
- //! Set the set of particles used - virtual void set_particles(const Particles &ps)=0; - //! Return true if the two particles are bonded virtual bool are_bonded(Particle *a, Particle *b) const =0; }; Index: kernel/include/IMP/score_states/QuadraticAllNonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/QuadraticAllNonbondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/QuadraticAllNonbondedListScoreState.h (working copy) @@ -1,49 +0,0 @@ -/** - * \file QuadraticAllNonbondedListScoreState.h - * \brief Allow iteration through pairs of a set of spheres. - * - * Copyright 2007-8 Sali Lab. All rights reserved. - */ - -#ifndef __IMP_QUADRATIC_ALL_NONBONDED_LIST_SCORE_STATE_H -#define __IMP_QUADRATIC_ALL_NONBONDED_LIST_SCORE_STATE_H - -#include "NonbondedListScoreState.h" -#include "../internal/kernel_version_info.h" -#include "QuadraticNonbondedListScoreState.h" - -#include <vector> -#include <limits> - -namespace IMP -{ - -//! This class maintains a list of non-bonded pairs of spheres -/** \note QuadraticBipartiteNonbondedListScoreState is basically an - implementation detail for performance analysis and should not - be used by end users. - */ -class IMPDLLEXPORT QuadraticAllNonbondedListScoreState: - public QuadraticNonbondedListScoreState -{ - typedef QuadraticNonbondedListScoreState P; - - //! \internal - void rebuild_nbl(Float cut); - -public: - /** - \param[in] ps A list of particles to use. - \param[in] radius The key to use to get the radius - */ - QuadraticAllNonbondedListScoreState(FloatKey radius, - const Particles &ps= Particles()); - ~QuadraticAllNonbondedListScoreState(); - IMP_SCORE_STATE(internal::kernel_version_info) - - void set_particles(const Particles &ps); -}; - -} // namespace IMP - -#endif /* __IMP_QUADRATIC_ALL_NONBONDED_LIST_SCORE_STATE_H */ Index: kernel/include/IMP/score_states/ChainBondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/ChainBondedListScoreState.h (revision 0) +++ kernel/include/IMP/score_states/ChainBondedListScoreState.h (revision 0) @@ -0,0 +1,44 @@ +/** + * \file ChainBondedListScoreState.h + * \brief Allow iteration through pairs of a set of atoms. + * + * Copyright 2007-8 Sali Lab. All rights reserved. + */ + +#ifndef __IMP_CHAIN_BONDED_LIST_SCORE_STATE_H +#define __IMP_CHAIN_BONDED_LIST_SCORE_STATE_H + +#include "BondedListScoreState.h" +#include "../decorators/bond_decorators.h" +#include "../internal/Vector.h" + +namespace IMP +{ + +//! Exclude consecutive particles in a chain. +/** \ingroup bond + */ +class IMPDLLEXPORT ChainBondedListScoreState: public BondedListScoreState +{ + std::vector<internal::Vector<Particle*> > chains_; + IntKey cik_; + unsigned int next_index_; +public: + //! Find bonds amongst the following points. + /** \param [in] ps The set of particles to use. + */ + ChainBondedListScoreState(); + virtual ~ChainBondedListScoreState(){} + + void add_chain(const Particles &ps); + + void clear_chains(); + + virtual bool are_bonded(Particle *a, Particle *b) const; + + virtual void update(); +}; + +} // namespace IMP + +#endif /* __IMP_BOND_DECORATOR_LIST_SCORE_STATE_H */ Index: kernel/include/IMP/score_states/AllNonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/AllNonbondedListScoreState.h (revision 524) +++ kernel/include/IMP/score_states/AllNonbondedListScoreState.h (working copy) @@ -9,76 +9,93 @@ #define __IMP_ALL_NONBONDED_LIST_SCORE_STATE_H
#include "NonbondedListScoreState.h" -#include "../internal/ParticleGrid.h" #include "../internal/kernel_version_info.h" -#include "MaxChangeScoreState.h" +#include "../internal/Vector.h"
-#include <vector> -#include <limits>
namespace IMP {
-//! This class maintains a list of non-bonded pairs of particles -/** \note If no value for the radius key is specified, all radii are - considered to be zero. +class MaxChangeScoreState; +namespace internal +{ +class ParticleGrid; +}
- \note The radius is currently assumed not to change. This could - be fixed later. +//! This class maintains a list of non-bonded pairs of spheres +/** To iterate through the list of pairs use the NonbondedListScoreState::begin, + NonbondedListScoreState::end functions.
- \todo The structure is slightly dumb about rebuilding and will - rebuild the whole list of any of the grids become invalidated. - This could be improved as each piece is computed separately (so - they could be cached). + The radius key can be the default key.
+ Changes in coordinates and radii are handled properly unless grid is used, + then changes in radii may not be handled properly. + \ingroup restraint */ class IMPDLLEXPORT AllNonbondedListScoreState: public NonbondedListScoreState { typedef NonbondedListScoreState P; + public: + //! What algorithm to use to perform the computations + enum Algorithm { + //! Check all pairs of particles to see if they are close enough + QUADRATIC, + GRID, + //! Sweep space looking for intersecting bounding boxes. + BBOX, + DEFAULT + }; protected: - //! \internal - struct Bin - { - internal::ParticleGrid *grid; - Float rmax; - Bin(): grid(NULL), rmax(-1){} - Bin(const Bin &o): grid(o.grid), rmax(o.rmax){} - }; - std::vector<Bin> bins_; + Algorithm a_; + internal::ObjectPointer<MaxChangeScoreState, true> mc_, mcr_; + internal::Vectorinternal::ParticleGrid* bins_;
+ void check_nbl() const; + //! \internal - void rebuild_nbl(Float cut); + void rebuild_nbl();
- void repartition_points(const Particles &ps, std::vector<Bin> &out);
- float side_from_r(float r) const; + // methods for grid + void grid_rebuild_nbl();
- void generate_nbl(const Bin &particle_bin, const Bin &grid_bin, float cut); + void grid_partition_points();
- void cleanup(std::vector<Bin> &bins); + void grid_generate_nbl(const internal::ParticleGrid *particle_bin, + const internal::ParticleGrid *grid_bin);
+ float grid_side_from_r(float r) const; public: - /** - \param[in] ps A list of particles to use. + /**\param[in] cutoff The distance cutoff to use. \param[in] radius The key to use to get the radius + \param[in] a Which algorithm to use. The default is the best. */ - AllNonbondedListScoreState(FloatKey radius, - const Particles &ps=Particles()); + AllNonbondedListScoreState(Float cutoff, + FloatKey radius, + Algorithm a= DEFAULT); + /**\param[in] cutoff The distance cutoff to use. + \param[in] radius The key to use to get the radius + \param[in] ps A list of particles to use. + \param[in] a Which algorithm to use. The default is the best. + */ + AllNonbondedListScoreState(Float cutoff, + FloatKey radius, + const Particles &ps, + Algorithm a= DEFAULT); ~AllNonbondedListScoreState(); - IMP_SCORE_STATE(internal::kernel_version_info) + IMP_SCORE_STATE(internal::kernel_version_info);
+ //! Add the particles and add them to the NBL + void add_particles(const Particles &ps); + //! Remove all particles + void clear_particles(); + //! Replace the set of particles void set_particles(const Particles &ps);
- //! Add a few particles to the nonbonded list - /** Note that this invalidates the nonbonded list. - \todo We could just add newly created pairs to the nonbonded list. - */ - void add_particles(const Particles &ps); - - //! Return a list of all the particles used - Particles get_particles() const; + /** If there is CGAL support, a more efficient algorithm BBOX can be used*/ + void set_algorithm(Algorithm a); };
} // namespace IMP Index: kernel/src/score_states/BondDecoratorListScoreState.cpp =================================================================== --- kernel/src/score_states/BondDecoratorListScoreState.cpp (revision 524) +++ kernel/src/score_states/BondDecoratorListScoreState.cpp (working copy) @@ -24,7 +24,7 @@ bonds_.clear(); for (unsigned int i=0; i< ps_.size(); ++i) { if (!ps_[i]->get_is_active()) continue; - BondedDecorator di= BondedDecorator::cast(ps_[i]); + BondedDecorator di(ps_[i]); ParticleIndex pi= ps_[i]->get_index(); for (unsigned int j=0; j< di.get_number_of_bonds(); ++j) { BondedDecorator dj= di.get_bonded(j); Index: kernel/src/score_states/NonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/NonbondedListScoreState.cpp (revision 524) +++ kernel/src/score_states/NonbondedListScoreState.cpp (working copy) @@ -15,22 +15,27 @@ namespace IMP {
-NonbondedListScoreState::NonbondedListScoreState(FloatKey rk): rk_(rk) +NonbondedListScoreState +::NonbondedListScoreState(Float cut, + FloatKey rk): rk_(rk), + cutoff_(cut), + nbl_is_valid_(false) { - last_cutoff_=-1; + slack_=10; + num_steps_=0; }
- - - - -void NonbondedListScoreState::propagate_particles(const Particles&ps) -{ - clear_nbl(); - for (BondedListScoreStateIterator bli= bonded_lists_begin(); - bli != bonded_lists_end(); ++bli) { - (*bli)->set_particles(ps); +Particles NonbondedListScoreState +::particles_with_radius(const Particles &in) const { + Particles ret; + if (rk_== FloatKey()) return ret; + ret.reserve(in.size()); + for (unsigned int i=0; i< in.size(); ++i) { + if (in[i]->has_attribute(rk_)) { + ret.push_back(in[i]); + } } + return ret; }
namespace internal @@ -45,7 +50,7 @@
} // namespace internal
-void NonbondedListScoreState::update() +bool NonbondedListScoreState::update(Float mc) { IMP_LOG(VERBOSE, "Updating non-bonded list" << std::endl); for (BondedListScoreStateIterator bli= bonded_lists_begin(); @@ -53,9 +58,21 @@ (*bli)->update(); }
- // if the list is not deleted, we need to scan for inactive particles - nbl_.erase(std::remove_if(nbl_.begin(), nbl_.end(), internal::HasInactive()), - nbl_.end()); + if (mc > slack_ || nbl_.size() ==0) { + num_steps_=0; + set_nbl_is_valid(false); + slack_= next_slack_; + rebuild_nbl(); + return true; + } else { + // if the list is not deleted, we need to scan for inactive particles + nbl_.erase(std::remove_if(nbl_.begin(), nbl_.end(), + internal::HasInactive()), + nbl_.end()); + ++num_steps_; + next_slack_= mc/num_steps_ * 100.0; + return false; + } }
void NonbondedListScoreState::show(std::ostream &out) const Index: kernel/src/score_states/QuadraticBipartiteNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/QuadraticBipartiteNonbondedListScoreState.cpp (revision 524) +++ kernel/src/score_states/QuadraticBipartiteNonbondedListScoreState.cpp (working copy) @@ -1,66 +0,0 @@ -/** - * \file QuadraticBipartiteNonbondedListScoreState.cpp - * \brief Allow iteration through pairs of a set of atoms. - * - * Copyright 2007-8 Sali Lab. All rights reserved. - */ - -#include "IMP/score_states/QuadraticBipartiteNonbondedListScoreState.h" -#include "IMP/decorators/XYZDecorator.h" -#include "IMP/score_states/MaxChangeScoreState.h" - -namespace IMP -{ - -QuadraticBipartiteNonbondedListScoreState -::QuadraticBipartiteNonbondedListScoreState(FloatKey rk, - const Particles &ps0, - const Particles &ps1): P(rk) -{ - set_particles(ps0, ps1); -} - - -QuadraticBipartiteNonbondedListScoreState -::QuadraticBipartiteNonbondedListScoreState(FloatKey rk): P(rk) -{ -} - -void QuadraticBipartiteNonbondedListScoreState::rebuild_nbl(float cut) -{ - IMP_LOG(TERSE, "Rebuilding QBNBL with lists of size " << na_ - << " and " << P::get_particles().size() -na_ - << " and cutoff " << cut << std::endl); - for (unsigned int i=0; i< na_; ++i) { - for (unsigned int j=na_; j < P::get_particles().size(); ++j) { - P::handle_nbl_pair(P::get_particles()[i], - P::get_particles()[j], - cut); - } - } - IMP_LOG(TERSE, "NBL has " << P::nbl_.size() << " pairs" << std::endl); -} - -void QuadraticBipartiteNonbondedListScoreState -::set_particles(const Particles &ps0, - const Particles &ps1) -{ - IMP_LOG(TERSE, "Setting QBNBLSS particles " << ps0.size() - << " and " << ps1.size() << std::endl); - Particles all(ps0); - all.insert(all.end(), ps1.begin(), ps1.end()); - P::set_particles(all); - na_= ps0.size(); -} - -void QuadraticBipartiteNonbondedListScoreState::update() -{ - P::update(); -} - -void QuadraticBipartiteNonbondedListScoreState::show(std::ostream &out) const -{ - out << "QuadraticBipartiteNonbondedList" << std::endl; -} - -} // namespace IMP Index: kernel/src/score_states/BipartiteNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/BipartiteNonbondedListScoreState.cpp (revision 0) +++ kernel/src/score_states/BipartiteNonbondedListScoreState.cpp (revision 0) @@ -0,0 +1,128 @@ +/** + * \file BipartiteNonbondedListScoreState.cpp + * \brief Bipartiteow iteration through pairs of a set of s. + * + * Copyright 2007-8 Sali Lab. Bipartite rights reserved. + */ + +#include "IMP/score_states/BipartiteNonbondedListScoreState.h" +#include "IMP/decorators/XYZDecorator.h" +#include "IMP/internal/utility.h" +#include "IMP/internal/bbox_nbl_helpers.h" + +#include <vector> + +namespace IMP +{ + +BipartiteNonbondedListScoreState +::BipartiteNonbondedListScoreState(Float cut, + FloatKey rk, + Algorithm a): + P(cut, rk), a_(a) +{ + FloatKeys ks; + ks.push_back(rk); + mc0_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + mc1_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + mcr_= new MaxChangeScoreState(ks); +} + +BipartiteNonbondedListScoreState +::BipartiteNonbondedListScoreState(Float cut, FloatKey rk, + const Particles &ps0, + const Particles &ps1, + Algorithm a): + P(cut, rk), a_(a) +{ + FloatKeys ks; + ks.push_back(rk); + mc0_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + mc1_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + mcr_= new MaxChangeScoreState(ks); + set_particles(ps0, ps1); +} + + +BipartiteNonbondedListScoreState::~BipartiteNonbondedListScoreState(){} + +void BipartiteNonbondedListScoreState::update() { + mc0_->update(); + mc0_->update(); + mcr_->update(); + Float mc= std::max(mc0_->get_max(), mc1_->get_max()); + if (P::update(mc+ mcr_->get_max())) { + mc0_->reset(); + mc1_->reset(); + mcr_->reset(); + } +} + +void BipartiteNonbondedListScoreState::process_sets(const Particles &p0, + const Particles &p1) { + if (a_== QUADRATIC) { + for (unsigned int j=0; j< p0.size(); ++j) { + for (unsigned int i=0; i< p1.size(); ++i) { + P::add_if_box_overlap(p1[i], p0[j]); + } + } + } +#ifdef IMP_USE_CGAL + else if (a_== BBOX) { + internal::bipartite_bbox_scan(p0, p1, P::get_radius_key(), + P::get_slack(), P::get_cutoff(), + internal::NBLAddPairIfNonbonded(this)); + + } +#endif + } + +void BipartiteNonbondedListScoreState::rebuild_nbl() { + IMP_LOG(TERSE, "Rebuilding QNBL with cutoff " + << P::get_cutoff() << std::endl); + process_sets(mc0_->get_particles(), mc1_->get_particles()); + P::set_nbl_is_valid(true); + IMP_LOG(TERSE, "NBL has " << P::number_of_nonbonded() + << " pairs" << std::endl); +} + +void BipartiteNonbondedListScoreState::set_particles(const Particles &ps0, + const Particles &ps1) { + mc0_->clear_particles(); + mc0_->add_particles(ps0); + mc1_->clear_particles(); + mc1_->add_particles(ps1); + mcr_->clear_particles(); + mcr_->add_particles(P::particles_with_radius(ps0)); + mcr_->add_particles(P::particles_with_radius(ps1)); + P::set_nbl_is_valid(false); +} + + +void BipartiteNonbondedListScoreState::add_particles_0(const Particles &ps) { + if (P::get_nbl_is_valid()) process_sets(ps, mc1_->get_particles()); + mc0_->add_particles(ps); + mcr_->add_particles(P::particles_with_radius(ps)); +} + +void BipartiteNonbondedListScoreState::add_particles_1(const Particles &ps) { + if (P::get_nbl_is_valid()) process_sets(ps, mc0_->get_particles()); + mc1_->add_particles(ps); + mcr_->add_particles(P::particles_with_radius(ps)); +} + +void BipartiteNonbondedListScoreState::clear_particles() { + mc0_->clear_particles(); + mc1_->clear_particles(); + mcr_->clear_particles(); + P::set_nbl_is_valid(false); + P::set_nbl_is_valid(true); +} + + +void BipartiteNonbondedListScoreState::show(std::ostream &out) const +{ + out << "BipartiteNonbondedListScoreState" << std::endl; +} + +} // namespace IMP Index: kernel/src/score_states/SConscript =================================================================== --- kernel/src/score_states/SConscript (revision 524) +++ kernel/src/score_states/SConscript (working copy) @@ -1,11 +1,10 @@ Import('env') - -files = ['NonbondedListScoreState.cpp', - 'MaxChangeScoreState.cpp', 'BondDecoratorListScoreState.cpp', - 'AllNonbondedListScoreState.cpp', - 'QuadraticBipartiteNonbondedListScoreState.cpp', - 'QuadraticAllNonbondedListScoreState.cpp', - 'QuadraticNonbondedListScoreState.cpp', 'GravityCenterScoreState.cpp'] - -files = [File(x) for x in files] +files=[] +files.append(File( 'AllNonbondedListScoreState.cpp' )) +files.append(File( 'BipartiteNonbondedListScoreState.cpp' )) +files.append(File( 'BondDecoratorListScoreState.cpp' )) +files.append(File( 'ChainBondedListScoreState.cpp' )) +files.append(File( 'GravityCenterScoreState.cpp' )) +files.append(File( 'MaxChangeScoreState.cpp' )) +files.append(File( 'NonbondedListScoreState.cpp' )) Return('files') Index: kernel/src/score_states/QuadraticAllNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/QuadraticAllNonbondedListScoreState.cpp (revision 524) +++ kernel/src/score_states/QuadraticAllNonbondedListScoreState.cpp (working copy) @@ -1,58 +0,0 @@ -/** - * \file QuadraticAllNonbondedListScoreState.cpp - * \brief Allow iteration through pairs of a set of s. - * - * Copyright 2007-8 Sali Lab. All rights reserved. - */ - -#include "IMP/score_states/QuadraticAllNonbondedListScoreState.h" -#include "IMP/decorators/XYZDecorator.h" - -#include <algorithm> - -namespace IMP -{ - - -QuadraticAllNonbondedListScoreState -::QuadraticAllNonbondedListScoreState(FloatKey radius, - const Particles &ps): P(radius) -{ - set_particles(ps); -} - -QuadraticAllNonbondedListScoreState::~QuadraticAllNonbondedListScoreState() -{ -} - -void QuadraticAllNonbondedListScoreState::set_particles(const Particles &ps) -{ - P::set_particles(ps); -} - - -void QuadraticAllNonbondedListScoreState::update() -{ - P::update(); -} - - -void QuadraticAllNonbondedListScoreState::rebuild_nbl(Float cut) -{ - IMP_LOG(TERSE, "Rebuilding QNBL with cutoff " << cut << std::endl); - const Particles &moving= P::get_particles(); - for (unsigned int j=0; j< moving.size(); ++j) { - for (unsigned int i=0; i< j; ++i) { - P::handle_nbl_pair(moving[i], moving[j], cut); - } - } - IMP_LOG(TERSE, "NBL has " << P::nbl_.size() << " pairs" << std::endl); -} - - -void QuadraticAllNonbondedListScoreState::show(std::ostream &out) const -{ - out << "QuadraticAllNonbondedListScoreState" << std::endl; -} - -} // namespace IMP Index: kernel/src/score_states/ChainBondedListScoreState.cpp =================================================================== --- kernel/src/score_states/ChainBondedListScoreState.cpp (revision 0) +++ kernel/src/score_states/ChainBondedListScoreState.cpp (revision 0) @@ -0,0 +1,64 @@ +/** + * \file ChainBondedListScoreState.cpp + * \brief Allow iteration through pairs of a set of atoms. + * + * Copyright 2007-8 Sali Lab. All rights reserved. + */ + +#include "IMP/score_states/ChainBondedListScoreState.h" + +#include <algorithm> +#include <sstream> + +namespace IMP +{ + +ChainBondedListScoreState::ChainBondedListScoreState() +{ + std::ostringstream oss; + oss << "ChainBLSS " << this; + cik_= IntKey(oss.str().c_str()); + next_index_=0; +} + +void ChainBondedListScoreState::update() +{ + IMP_LOG(TERSE, "Updating ChainBondedList assumed to be static" << std::endl); +} + +void ChainBondedListScoreState::clear_chains() +{ + for (unsigned int i=0; i< chains_.size(); ++i) { + for (unsigned int j=0; j< chains_[i].size(); ++j) { + chains_[i][j]->set_value(cik_, -1); + } + } + chains_.clear(); + next_index_=0; +} + +void ChainBondedListScoreState::add_chain(const Particles &ps) +{ + chains_.push_back(internal::Vector<Particle*>(ps.begin(), ps.end())); + for (unsigned int i=0; i< chains_.back().size(); ++i) { + Particle *p= chains_.back()[i]; + if (p->has_attribute(cik_)) { + p->set_value(cik_, next_index_); + } else { + p->add_attribute(cik_, next_index_); + } + ++next_index_; + } + ++next_index_; +} + + +bool ChainBondedListScoreState::are_bonded(Particle *a, Particle *b) const +{ + if (!a->has_attribute(cik_) || !b->has_attribute(cik_)) return false; + int ia= a->get_value(cik_); + int ib= b->get_value(cik_); + return std::abs(ia-ib) ==1; +} + +} // namespace IMP Index: kernel/src/score_states/MaxChangeScoreState.cpp =================================================================== --- kernel/src/score_states/MaxChangeScoreState.cpp (revision 524) +++ kernel/src/score_states/MaxChangeScoreState.cpp (working copy) @@ -31,12 +31,15 @@ IMP_LIST_IMPL(MaxChangeScoreState, Particle, particle, Particle*, {for (unsigned int i=0; i< keys_.size(); ++i) { IMP_check(obj->has_attribute(keys_[i]), - "Particle missing needed attribute", + "Particle missing needed attribute " << keys_[i] + << obj, ValueException("Particle missing attribute")); }; for (unsigned int i=0; i< origkeys_.size(); ++i) { - obj->add_attribute(origkeys_[i], - obj->get_value(keys_[i]), false); + if (!obj->has_attribute(origkeys_[i])) { + obj->add_attribute(origkeys_[i], + obj->get_value(keys_[i]), false); + } } }, {reset();});
Index: kernel/src/score_states/AllNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/AllNonbondedListScoreState.cpp (revision 524) +++ kernel/src/score_states/AllNonbondedListScoreState.cpp (working copy) @@ -7,101 +7,170 @@
#include "IMP/score_states/AllNonbondedListScoreState.h" #include "IMP/decorators/XYZDecorator.h" +#include "IMP/internal/utility.h" +#include "IMP/internal/bbox_nbl_helpers.h"
-#include <algorithm> +#include <vector>
namespace IMP {
-static unsigned int min_grid_size=20; +// Turn the default into an actuall algorithm and work around missing algorithms +static AllNonbondedListScoreState::Algorithm +translate_algorithm(AllNonbondedListScoreState::Algorithm a){ +#ifdef IMP_USE_CGAL + switch (a) { + case AllNonbondedListScoreState::DEFAULT: + return AllNonbondedListScoreState::BBOX; + default: + return a; + } +#else + switch (a) { + case AllNonbondedListScoreState::BBOX: + IMP_WARN("AllNonbondedListScoreState::BBOX requires CGAL support. " + << "GRID used instead." << std::endl); + case AllNonbondedListScoreState::DEFAULT: + return AllNonbondedListScoreState::GRID; + default: + return a; + } +#endif +}
-AllNonbondedListScoreState -::AllNonbondedListScoreState(FloatKey radius, const Particles &ps): - P(radius) +AllNonbondedListScoreState::AllNonbondedListScoreState(Float cut, + FloatKey rk, + const Particles &ps, + Algorithm a): + P(cut, rk), a_(translate_algorithm(a)) { - set_particles(ps); + FloatKeys ks; + ks.push_back(rk); + mc_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + mcr_= new MaxChangeScoreState(ks); + add_particles(ps); }
- -AllNonbondedListScoreState::~AllNonbondedListScoreState() +AllNonbondedListScoreState::AllNonbondedListScoreState(Float cut, + FloatKey rk, + Algorithm a): + P(cut, rk), a_(translate_algorithm(a)) { - cleanup(bins_); + FloatKeys ks; + ks.push_back(rk); + mc_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + mcr_= new MaxChangeScoreState(ks); }
-float AllNonbondedListScoreState::side_from_r(float r) const -{ - if (r==0) return 1; - else return r*1.6; +AllNonbondedListScoreState::~AllNonbondedListScoreState(){ }
-Particles AllNonbondedListScoreState::get_particles() const -{ - Particles ret; - for (unsigned int i=0; i< bins_.size(); ++i) { - ret.insert(ret.end(), bins_[i].grid->get_particles().begin(), - bins_[i].grid->get_particles().end()); +void AllNonbondedListScoreState::update() { + mc_->update(); + mcr_->update(); + Float mc=mc_->get_max()+ mcr_->get_max(); + if (P::update(mc)) { + mc_->reset(); } - return ret; }
-void AllNonbondedListScoreState::set_particles(const Particles &ps) -{ - NonbondedListScoreState::clear_nbl(); - cleanup(bins_); - repartition_points(ps, bins_); -}
-void AllNonbondedListScoreState::add_particles(const Particles &ps) -{ - if (bins_.empty()) set_particles(ps); - else { -#ifndef NDEBUG - for (unsigned int i=0; i< ps.size(); ++i) { - for (unsigned int j=0; j< bins_.size(); ++j) { - for (unsigned int k=0; k< bins_[j].grid->get_particles().size(); ++k) { - IMP_assert(ps[i] != bins_[j].grid->get_particles()[k], - "Can't add a particle that is already there"); - - IMP_assert(ps[i]->get_index() - != bins_[j].grid->get_particles()[k]->get_index(), - "Same particle index, different particles."); - } +void AllNonbondedListScoreState::rebuild_nbl() { + IMP_LOG(TERSE, "Rebuilding QNBL with cutoff " + << P::get_cutoff() << std::endl); + if (a_== QUADRATIC) { + const Particles &moving= mc_->get_particles(); + for (unsigned int j=0; j< moving.size(); ++j) { + for (unsigned int i=0; i< j; ++i) { + P::add_if_box_overlap(moving[i], moving[j]); } } + } else if (a_ == GRID) { + grid_rebuild_nbl(); + } else if (a_== BBOX) { + internal::bbox_scan(mc_->get_particles(), P::get_radius_key(), + P::get_slack(), P::get_cutoff(), + internal::NBLAddPairIfNonbonded(this)); + + } else { + IMP_failure("Bad algorithm in AllNBL::rebuild", ErrorException()); + } + set_nbl_is_valid(true); +#ifndef NDEBUG + check_nbl(); #endif + IMP_LOG(TERSE, "NBL has " << P::number_of_nonbonded() + << " pairs" << std::endl); +}
- for (unsigned int i=0; i< ps.size(); ++i) { - float r= P::get_radius(ps[i]); - bool found=false; - for (unsigned int j=0; j< bins_.size(); ++j) { - if (bins_[j].rmax >= r) { - found=true; - IMP_LOG(VERBOSE, "Adding particle " - << ps[i]->get_index() << " to bin " << j << std::endl); - bins_[j].grid->add_particle(ps[i]); - break; +void AllNonbondedListScoreState::set_particles(const Particles &ps) { + mc_->clear_particles(); + mc_->add_particles(ps); + mcr_->clear_particles(); + mcr_->add_particles(particles_with_radius(ps)); + P::set_nbl_is_valid(false); +} + + +void AllNonbondedListScoreState::add_particles(const Particles &ps) { + if (P::get_nbl_is_valid()) { + if (a_== QUADRATIC || a_ == GRID) { + const Particles &moving= mc_->get_particles(); + for (unsigned int j=0; j< moving.size(); ++j) { + for (unsigned int i=0; i< ps.size(); ++i) { + P::add_if_box_overlap(ps[i], moving[j]); } } - if (!found) { - bins_.back().rmax=r; - bins_.back().grid->add_particle(ps[i]); - } + } else if (a_== BBOX) { + internal::bipartite_bbox_scan(mc_->get_particles(), ps, + P::get_radius_key(), + P::get_slack(), P::get_cutoff(), + internal::NBLAddPairIfNonbonded(this)); } } - IMP_LOG(TERSE, "Destroying nbl in list due to additions"<< std::endl); - NonbondedListScoreState::clear_nbl(); + mcr_->add_particles(particles_with_radius(ps)); + mc_->add_particles(ps); }
+void AllNonbondedListScoreState::clear_particles() { + mc_->clear_particles(); + mcr_->clear_particles(); + P::set_nbl_is_valid(false); + P::set_nbl_is_valid(true); +}
-void AllNonbondedListScoreState::repartition_points(const Particles &ps, - std::vector<Bin> &out) +void AllNonbondedListScoreState::show(std::ostream &out) const { - cleanup(out); - if (ps.empty()) return; + out << "AllNonbondedListScoreState" << std::endl; +} + + +void AllNonbondedListScoreState::set_algorithm(Algorithm a) { + a_=translate_algorithm(a); + if (a_!= GRID) { + bins_.clear(); + } +} + + +// methods for grid + +static unsigned int min_grid_size=20; + +float AllNonbondedListScoreState::grid_side_from_r(float r) const +{ + if (r==0) return 1; + else return r*1.6; +} + +void AllNonbondedListScoreState::grid_partition_points() +{ + bins_.clear(); + if (mc_->get_particles().empty()) return; float minr=std::numeric_limits<float>::max(), maxr=0; - for (unsigned int i=0; i< ps.size(); ++i) { - float r= P::get_radius(ps[i]); + for (unsigned int i=0; i< mc_->get_particles().size(); ++i) { + float r= P::get_radius(mc_->get_particles()[i]); if ( r > maxr) maxr=r; if ( r > 0 && r < minr) minr=r; } @@ -116,13 +185,13 @@ cuts.push_back(2*maxr);
std::vector<Particles> ops(cuts.size()); - for (unsigned int i=0; i< ps.size(); ++i) { - float r= P::get_radius(ps[i]); + for (unsigned int i=0; i< mc_->get_particles().size(); ++i) { + float r= P::get_radius(mc_->get_particles()[i]); bool found=false; for (unsigned int j=0; ; ++j) { IMP_assert(j< cuts.size(), "Internal error in ASNBLSS"); if (cuts[j] >= r) { - ops[j].push_back(ps[i]); + ops[j].push_back(mc_->get_particles()[i]); found=true; break; } @@ -138,156 +207,110 @@ } for (unsigned int i=0; i< cuts.size(); ++i) { if (ops[i].empty()) continue; - out.push_back(Bin()); float rmax=0; for (unsigned int j=0; j< ops[i].size(); ++j) { rmax= std::max(rmax, P::get_radius(ops[i][j])); } - out.back().rmax= rmax; - internal::ParticleGrid *pg - = new internal::ParticleGrid(side_from_r(out.back().rmax)); - out.back().grid= pg; - out.back().grid->add_particles(ops[i]); + bins_.push_back(new internal::ParticleGrid(grid_side_from_r(rmax), ops[i])); } - IMP_LOG(VERBOSE, "Created " << out.size() << " grids" << std::endl); - for (unsigned int i=0; i< out.size(); ++i) { - IMP_LOG(VERBOSE, out[i].rmax - << ": " << *out[i].grid << std::endl); + IMP_LOG(VERBOSE, "Created " << bins_.size() << " grids" << std::endl); + for (unsigned int i=0; i< bins_.size(); ++i) { + IMP_LOG(VERBOSE, *bins_[i] << std::endl); } - -#ifndef NDEBUG - Particles all; - for (unsigned int i=0; i< out.size(); ++i) { - all.insert(all.end(), out[i].grid->get_particles().begin(), - out[i].grid->get_particles().end()); - } - std::sort(all.begin(), all.end()); - Particles::iterator it = std::unique(all.begin(), all.end()); - IMP_assert(it == all.end(), "Duplicate particles " << all.size()); - IMP_assert(all.size() == ps.size(), "Wrong number of particles at end " - << all.size() << " " << ps.size()); -#endif }
-void AllNonbondedListScoreState::cleanup(std::vector<Bin> &bins) -{ - for (unsigned int i=0; i< bins.size(); ++i) { - delete bins[i].grid; - } - bins.clear(); -}
-void AllNonbondedListScoreState::update() +void AllNonbondedListScoreState +::grid_generate_nbl(const internal::ParticleGrid *particle_bin, + const internal::ParticleGrid *grid_bin) { - IMP_LOG(TERSE, "Updating nonbonded list"<< std::endl); - NonbondedListScoreState::update(); - bool bad=false; - for (unsigned int i=0; i< bins_.size(); ++i) { - bad = bins_[i].grid->update() || bad; - IMP_LOG(VERBOSE, bins_[i].rmax << std::endl << *bins_[i].grid << std::endl); - } - if (bad) { - IMP_LOG(TERSE, "Destroying nbl in list"<< std::endl); - NonbondedListScoreState::clear_nbl(); - } -} - -void AllNonbondedListScoreState::generate_nbl(const Bin &particle_bin, - const Bin &grid_bin, - float cut) -{ - IMP_CHECK_OBJECT(particle_bin.grid); - IMP_CHECK_OBJECT(grid_bin.grid); - IMP_LOG(VERBOSE, "Generate nbl for " << particle_bin.rmax - << " and " << grid_bin.rmax << std::endl); - for (unsigned int k=0; k< particle_bin.grid->get_particles().size(); ++k) { - Particle *p= particle_bin.grid->get_particles()[k]; - NonbondedListScoreState::AddToNBL f(this, p); + IMP_CHECK_OBJECT(particle_bin); + IMP_CHECK_OBJECT(grid_bin); + IMP_LOG(VERBOSE, "Generate nbl for pair " << std::endl); + for (internal::ParticleGrid::ParticleVoxelIterator + it= particle_bin->particle_voxels_begin(); + it != particle_bin->particle_voxels_end(); ++it) { + Particle *p= it->first; + internal::NBLAddIfNonbonded f(this, p); XYZDecorator d= XYZDecorator::cast(p); internal::ParticleGrid::VirtualIndex index - = grid_bin.grid->get_virtual_index(Vector3D(d.get_x(), - d.get_y(), - d.get_z())); + = grid_bin->get_virtual_index(Vector3D(d.get_x(), + d.get_y(), + d.get_z())); IMP_LOG(VERBOSE, "Searching for " << p->get_index() - << " from " << index - << " of bin " << grid_bin.rmax << std::endl); - grid_bin.grid->apply_to_nearby(f, index, - cut+2*particle_bin.rmax + grid_bin.rmax, - false); + << " from " << index << std::endl); + grid_bin->apply_to_nearby(f, index, + P::get_cutoff() + 2*P::get_slack(), + false); }
}
-void AllNonbondedListScoreState::rebuild_nbl(Float cut) +void AllNonbondedListScoreState::grid_rebuild_nbl() { - IMP_LOG(TERSE, "Rebuilding NBL with " << bins_.size() << " bins" - << " and cutoff " << cut << std::endl); + IMP_LOG(TERSE, "Rebuilding NBL with Grid and cutoff " + << P::get_cutoff() << std::endl ); + grid_partition_points(); + for (unsigned int i=0; i< bins_.size(); ++i) { for (unsigned int j=i+1; j< bins_.size(); ++j) { - generate_nbl(bins_[i], bins_[j], cut); + grid_generate_nbl(bins_[i], bins_[j]); }
internal::ParticleGrid::Index last_index; for (internal::ParticleGrid::ParticleVoxelIterator it - = bins_[i].grid->particle_voxels_begin(); - it != bins_[i].grid->particle_voxels_end(); ++it) { + = bins_[i]->particle_voxels_begin(); + it != bins_[i]->particle_voxels_end(); ++it) { IMP_LOG(VERBOSE, "Searching with particle " << it->first->get_index() << std::endl); - NonbondedListScoreState::AddToNBL f(this, it->first); - bins_[i].grid->apply_to_nearby(f, it->second, - cut+3*bins_[i].rmax, + internal::NBLAddIfNonbonded f(this, it->first); + bins_[i]->apply_to_nearby(f, it->second, + P::get_cutoff()+2*P::get_slack(), true); + if (it->second != last_index) { + internal::NBLAddPairIfNonbonded fp(this); IMP_LOG(VERBOSE, "Searching in " << it->second << std::endl); - bins_[i].grid->apply_to_cell_pairs(f, it->second); + bins_[i]->apply_to_cell_pairs(fp, it->second); } last_index=it->second; } }
- IMP_LOG(TERSE, "NBL has " << P::nbl_.size() << " pairs" << std::endl); + bins_.clear(); + IMP_LOG(TERSE, "NBL has " << P::number_of_nonbonded() + << " pairs" << std::endl); +}
-#ifndef NDEBUG - { - Particles ps; - for (unsigned int i=0; i< bins_.size(); ++i) { - if (bins_[i].grid) { - ps.insert(ps.end(), bins_[i].grid->get_particles().begin(), - bins_[i].grid->get_particles().end()); - } - } - for (unsigned int i=0; i< ps.size(); ++i) { - XYZDecorator di= XYZDecorator::cast(ps[i]); - for (unsigned int j=0; j< i; ++j) { - XYZDecorator dj= XYZDecorator::cast(ps[j]); - IMP_assert(ps[i] != ps[j], "Duplicate particles in grid"); - if (distance(di, dj) - P::get_radius(ps[i]) - P::get_radius(ps[j]) - <= cut && !are_bonded(ps[i], ps[j])) { - bool found=false; - for (NonbondedIterator nit= nbl_.begin(); - nit != nbl_.end(); ++nit) { - if (nit->first == ps[i] && nit->second == ps[j] - || nit->first == ps[j] && nit->second == ps[i]) { - IMP_assert(!found, "Entry is in list twice"); - found=true; - } + +// debugging +void AllNonbondedListScoreState::check_nbl() const +{ + const Particles &ps= mc_->get_particles(); + for (unsigned int i=0; i< ps.size(); ++i) { + XYZDecorator di= XYZDecorator::cast(ps[i]); + for (unsigned int j=0; j< i; ++j) { + XYZDecorator dj= XYZDecorator::cast(ps[j]); + IMP_assert(ps[i] != ps[j], "Duplicate particles in grid"); + if (distance(di, dj) - P::get_radius(ps[i]) - P::get_radius(ps[j]) + <= P::get_cutoff() && !are_bonded(ps[i], ps[j])) { + bool found=false; + for (NonbondedIterator nit= P::nonbonded_begin(); + nit != P::nonbonded_end(); ++nit) { + if (nit->first == ps[i] && nit->second == ps[j] + || nit->first == ps[j] && nit->second == ps[i]) { + IMP_assert(!found, "Entry is in list twice"); + found=true; } - IMP_assert(found, "Nonbonded list is missing " - << ps[i]->get_index() << " " << di - << " and " << ps[j]->get_index() << " " - << dj << std::endl); } + IMP_assert(found, "Nonbonded list is missing " + << ps[i]->get_index() << " " << di + << " and " << ps[j]->get_index() << " " + << dj << std::endl); } } } -#endif }
- -void AllNonbondedListScoreState::show(std::ostream &out) const -{ - out << "AllNonbondedListScoreState" << std::endl; -} - } // namespace IMP Index: kernel/src/score_states/QuadraticNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/QuadraticNonbondedListScoreState.cpp (revision 524) +++ kernel/src/score_states/QuadraticNonbondedListScoreState.cpp (working copy) @@ -1,60 +0,0 @@ -/** - * \file QuadraticNonbondedListScoreState.cpp - * \brief Allow iteration through pairs of a set of s. - * - * Copyright 2007-8 Sali Lab. All rights reserved. - */ - -#include "IMP/score_states/QuadraticNonbondedListScoreState.h" -#include "IMP/decorators/XYZDecorator.h" - -#include <algorithm> - -namespace IMP -{ - -QuadraticNonbondedListScoreState -::QuadraticNonbondedListScoreState(FloatKey radius): - P(radius), slack_(.1) -{ - mc_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); -} - -QuadraticNonbondedListScoreState::~QuadraticNonbondedListScoreState() -{ -} - - -void QuadraticNonbondedListScoreState::update() -{ - // placeholder to do tuning of slack - NonbondedListScoreState::update(); - if (mc_->get_max() > slack_) { - NonbondedListScoreState::clear_nbl(); - mc_->reset(); - } -} - -void QuadraticNonbondedListScoreState -::handle_nbl_pair( Particle *a, Particle *b, - float d) -{ - XYZDecorator da= XYZDecorator::cast(a); - XYZDecorator db= XYZDecorator::cast(b); - float ra= P::get_radius(a); - float rb= P::get_radius(b); - for (unsigned int i=0; i< 3; ++i) { - float delta=std::abs(da.get_coordinate(i) - db.get_coordinate(i)); - if (delta -ra -rb > d-slack_) { - IMP_LOG(VERBOSE, "Pair " << a->get_index() - << " and " << b->get_index() << " rejected on coordinate " - << i << std::endl); - return ; - } - } - IMP_LOG(VERBOSE, "Adding pair " << a->get_index() - << " and " << b->get_index() << std::endl); - P::add_to_nbl(a, b); -} - -} // namespace IMP Index: kernel/include/IMP/internal/ParticleGrid.h =================================================================== --- kernel/include/IMP/internal/ParticleGrid.h (revision 524) +++ kernel/include/IMP/internal/ParticleGrid.h (working copy) @@ -25,26 +25,18 @@ // don't need ref counting since mc_ has the same set of points typedef internal::Grid3D<Particles> Grid; Grid grid_; - internal::ObjectPointer<MaxChangeScoreState, true> mc_; Float target_voxel_side_; - bool grid_valid_;
- void build_grid(); + void build_grid(const Particles &ps); void audit_particles(const Particles &ps) const; void add_particle_to_grid(Particle *p); public: ParticleGrid(); //! suggested grid edge size. - ParticleGrid(Float sz); + ParticleGrid(Float sz, const Particles &ps);
Float get_voxel_size() const {return target_voxel_side_;}
- void add_particles(const Particles &ps); - void add_particle(Particle *p); - void clear_particles(); - const Particles& get_particles() const {return mc_->get_particles();} - bool update(); - void show(std::ostream &out) const;
typedef Grid::VirtualIndex VirtualIndex; Index: kernel/src/internal/ParticleGrid.cpp =================================================================== --- kernel/src/internal/ParticleGrid.cpp (revision 524) +++ kernel/src/internal/ParticleGrid.cpp (working copy) @@ -18,19 +18,20 @@
static const int target_cell_occupancy=10;
-ParticleGrid::ParticleGrid(float tvs): target_voxel_side_(tvs), - grid_valid_(false) + ParticleGrid::ParticleGrid(float tvs, + const Particles &ps): target_voxel_side_(tvs) { IMP_assert(tvs >0, "Target voxel edge size must be positive"); - mc_= new MaxChangeScoreState(XYZDecorator::get_xyz_keys()); + build_grid(ps); }
-ParticleGrid::ParticleGrid(): target_voxel_side_(0), grid_valid_(0) +ParticleGrid::ParticleGrid(): target_voxel_side_(0) { }
-void ParticleGrid::build_grid() +void ParticleGrid::build_grid(const Particles &ps) { + audit_particles(ps); IMP_LOG(TERSE, "Creating nonbonded grid..." << std::flush); float mn[3]= {std::numeric_limits<float>::max(), std::numeric_limits<float>::max(), @@ -38,19 +39,19 @@ float mx[3]={-std::numeric_limits<float>::max(), -std::numeric_limits<float>::max(), -std::numeric_limits<float>::max()}; - for (unsigned int i = 0; i < mc_->get_particles().size(); ++i) { - XYZDecorator d= XYZDecorator::cast(mc_->get_particles()[i]); + for (unsigned int i = 0; i < ps.size(); ++i) { + XYZDecorator d(ps[i]); for (unsigned int j=0; j<3; ++j) { if (d.get_coordinate(j)< mn[j]) mn[j]= d.get_coordinate(j); if (d.get_coordinate(j)> mx[j]) mx[j]= d.get_coordinate(j); } } - if (!mc_->get_particles().empty()) { + if (!ps.empty()) { // keep the grid size sane if things blow up float maxdim= std::max(mx[0]-mn[0], std::max(mx[1]-mn[1], mx[2]-mn[2])); float vx= std::pow(static_cast<float>(target_cell_occupancy *(maxdim*maxdim*maxdim - /mc_->get_particles().size())), + /ps.size())), .3333f); if (vx > target_voxel_side_) { IMP_LOG(VERBOSE, "Overroade target side of " << target_voxel_side_ @@ -62,89 +63,16 @@ Vector3D(mn[0], mn[1], mn[2]), Vector3D(mx[0], mx[1], mx[2]), Particles()); - for (unsigned int i = 0; i < mc_->get_particles().size(); ++i) { - XYZDecorator d= XYZDecorator::cast(mc_->get_particles()[i]); + for (unsigned int i = 0; i < ps.size(); ++i) { + XYZDecorator d(ps[i]); Vector3D v(d.get_x(), d.get_y(), d.get_z()); - grid_.get_voxel(grid_.get_index(v)).push_back(mc_->get_particles()[i]); + grid_.get_voxel(grid_.get_index(v)).push_back(ps[i]); } - grid_valid_=true; - mc_->reset(); IMP_LOG(TERSE, "done." << std::endl); }
-void ParticleGrid::add_particles(const Particles &ps) -{ - audit_particles(ps); - mc_->add_particles(ps); - for (unsigned int i=0; i< ps.size(); ++i) { - if (grid_valid_) { - add_particle_to_grid(ps[i]); - } - } -}
-void ParticleGrid::add_particle(Particle *p) -{ - Particles ps(1, p); - audit_particles(ps); - mc_->add_particle(p); - - if (grid_valid_) { - add_particle_to_grid(p); - } -} - -void ParticleGrid::add_particle_to_grid(Particle *p) -{ - IMP_assert(grid_valid_, "Bad call of add particle to grid"); - XYZDecorator d= XYZDecorator::cast(p); - Vector3D v(d.get_x(), d.get_y(), d.get_z()); - Grid::VirtualIndex vi= grid_.get_virtual_index(v); - Grid::Index gi= grid_.get_index(vi); - if (gi== Grid::Index()) { - IMP_LOG(TERSE, "Adding particle off grid invalidates it " - << v << " " << vi << std::endl); - grid_valid_=false; - grid_ = Grid(); - } else { - grid_.get_voxel(gi).push_back(p); - } -} - -void ParticleGrid::clear_particles() -{ - mc_->clear_particles(); -} - -bool ParticleGrid::update() -{ - bool ret; - if (!grid_valid_ || mc_->get_max() > target_voxel_side_) { - IMP_LOG(TERSE, "Rebuilding particle grid\n"); - build_grid(); - ret= true; - } else { - IMP_LOG(TERSE, "Removing inactive particles\n"); - for (Grid::DataIterator dit= grid_.data_begin(); dit != grid_.data_end(); - ++dit) { - remove_inactive_particles(*dit); - } - ret= false; - } - unsigned int ssz=0; - for (Grid::DataIterator dit= grid_.data_begin(); dit != grid_.data_end(); - ++dit) { - ssz+= dit->size(); - } - // do this last since it has the ref counts - mc_->update(); - - IMP_assert(ssz== mc_->number_of_particles(), "Particle mismatch in PG"); - - return ret; -} - void ParticleGrid::audit_particles(const Particles &ps) const { for (unsigned int i=0; i< ps.size(); ++i) {
Daniel Russel wrote: > Frido was complaining that the nbl was too slow, so I thought I would > check in my local changes. The big thing is that the fastest > implementation I have uses the CGAL library.
All patches to IMP need to be reviewed before they are checked in. Obviously, I am not going to do this while I'm on vacation, so unless somebody else with some understanding of the code volunteers to check through these patches, they'll have to wait until I'm back in the lab.
I don't think we can use CGAL. It is certainly available for Fedora, but we build on a lot of other platforms too. And my understanding is that large chunks of it are QPL licensed, which is certainly incompatible with the IMP license. If we did use it, we'd have to be very careful to use only the LGPL parts. So I'd rather we didn't use it at all.
Your timings suggest that the current grid nonbonded list implementation is not O(N). Why not? It seems like it should be from my understanding of the code.
Ben
O(N) is not is irrelevant. The constants are what matters and they were bad. The code I submitted is better, but still not as fast as the CGAL one.
On Apr 26, 2008, at 10:56 AM, Ben Webb wrote:
> Daniel Russel wrote: >> Frido was complaining that the nbl was too slow, so I thought I would >> check in my local changes. The big thing is that the fastest >> implementation I have uses the CGAL library. > > All patches to IMP need to be reviewed before they are checked in. > Obviously, I am not going to do this while I'm on vacation, so unless > somebody else with some understanding of the code volunteers to check > through these patches, they'll have to wait until I'm back in the lab. Not that we could do anything even if we did review them...
Aren't you back now?
> I don't think we can use CGAL. It is certainly available for Fedora, > but > we build on a lot of other platforms too. And my understanding is that > large chunks of it are QPL licensed, which is certainly incompatible > with the IMP license. If we did use it, we'd have to be very careful > to > use only the LGPL parts. So I'd rather we didn't use it at all.
I thought LGPL could be linked against anything. And I thought IMP was going LGPL for exactly this sort of reason. It was a while ago, so I don't remember the final decision.
Daniel Russel wrote: > O(N) is not is irrelevant. The constants are what matters and they > were bad. The code I submitted is better, but still not as fast as the > CGAL one.
I must be misunderstanding your benchmark, in that case. You say: Grid For 50 particles got 3.63301706314 For 100 particles got 3.84657287598 For 1000 particles got 6.80274581909 For 10000 particles got 11.4696931839
and that the results are normalized by the number of particles. Surely if it were O(N) you'd get about 3.63 regardless of the number of particles, since by 'normalized' you mean these numbers are already divided by N?
>> All patches to IMP need to be reviewed before they are checked in. > Not that we could do anything even if we did review them...
If Keren, for example, were to review your patch and say it looks OK, I could check it in very quickly even over a dodgy dialup connection. Or others can have commit rights. It just doesn't make sense for the same person writing the code to commit it.
> Aren't you back now?
Monday. Today is a weekend.
> I thought LGPL could be linked against anything. And I thought IMP was > going LGPL for exactly this sort of reason. It was a while ago, so I > don't remember the final decision.
Nope, the other way around: anything can be linked to LGPL. We still have to respect the licenses of our dependencies. LGPL is what I proposed for IMP, but Andrej has not yet made a decision on this score. I proposed LGPL so that others could write their own extension modules and not be forced to make them open source.
Ben
On Apr 26, 2008, at 1:38 PM, Ben Webb wrote:
> Daniel Russel wrote: >> O(N) is not is irrelevant. The constants are what matters and they >> were bad. The code I submitted is better, but still not as fast as >> the >> CGAL one. > > I must be misunderstanding your benchmark, in that case. Typo. I meant "O(N) or not is irrelevant" as the constants are what really matters. But yes, as I mentioned before, supporting different sized particles makes it not O(N) as there is a term to handle the spread in radii.
> Nope, the other way around: anything can be linked to LGPL. We still > have to respect the licenses of our dependencies. LGPL is what I > proposed for IMP, but Andrej has not yet made a decision on this > score. > I proposed LGPL so that others could write their own extension modules > and not be forced to make them open source. Yes and anything can be linked against QPL, from my understanding, so I don't see the problem. What am I missing?
On Apr 26, 2008, at 10:56 AM, Ben Webb wrote:
> Daniel Russel wrote: >> Frido was complaining that the nbl was too slow, so I thought I would >> check in my local changes. The big thing is that the fastest >> implementation I have uses the CGAL library. > > All patches to IMP need to be reviewed before they are checked in. > Obviously, I am not going to do this while I'm on vacation, so unless > somebody else with some understanding of the code volunteers to check > through these patches, they'll have to wait until I'm back in the lab. > > I don't think we can use CGAL. It is certainly available for Fedora, > but > we build on a lot of other platforms too. It is tested on quite a variety of platforms. More than IMP, but I have not checked if it is a superset or not. And it doesn't really matter if a given platform is not supported. That platform will just use a slower algorithm.
Daniel Russel wrote: > On Apr 26, 2008, at 10:56 AM, Ben Webb wrote: >> I don't think we can use CGAL. It is certainly available for Fedora, >> but >> we build on a lot of other platforms too. > It is tested on quite a variety of platforms. More than IMP, but I > have not checked if it is a superset or not.
Indeed, but I think you missed my point: in order for us to test IMP with CGAL on all the platforms we build on, somebody (i.e. me) has to install CGAL on those platforms. I'm reluctant to do that unless we really need it. What do others think? Does anybody need CGAL for anything else? Requiring it just for a nonbonded list seems overkill.
Ben
On Apr 26, 2008, at 1:28 PM, Ben Webb wrote:
> Daniel Russel wrote: >> On Apr 26, 2008, at 10:56 AM, Ben Webb wrote: >>> I don't think we can use CGAL. It is certainly available for Fedora, >>> but >>> we build on a lot of other platforms too. >> It is tested on quite a variety of platforms. More than IMP, but I >> have not checked if it is a superset or not. > > Indeed, but I think you missed my point: in order for us to test IMP > with CGAL on all the platforms we build on, somebody (i.e. me) has to > install CGAL on those platforms. I'm reluctant to do that unless we > really need it. What do others think? Does anybody need CGAL for > anything else? Requiring it just for a nonbonded list seems overkill. >
CGAL might be very useful for various tasks such as grid manipulation, motion planning, dimensionality reduction and more. However, I really do not think it should be part of IMP kernel , which should provide basic functionality common to most tasks.
Two points: 1. We should have definition of what goes or does not go into the kernel - I think that now pretty much everything is part of the kernel ( other the embed lib) 2. Daniel - can you please elaborate more on the algorithmic differences between your "improved implementation" and the CGAL one? I can not help wondering why a simple geometric hashing (~5 c++ classes) can not help here.
On Apr 28, 2008, at 10:14 AM, Keren Lasker wrote: > CGAL might be very useful for various tasks such as grid manipulation, > motion planning, dimensionality reduction and more. > However, I really do not think it should be part of IMP kernel , > which should provide basic functionality common to most tasks. I am not saying CGAL should be part of the kernel. I am saying that the kernel should use CGAL (and any other external library that is easily available and saves us work). Anything which has builds for linux, mac os and windows covers basically all users should probably be fair game as a dependency. Some libraries will provide core functionality (like boost) and so IMP can't be build without them, while others, such as CGAL currently, simply improve IMP and so are optional.
> Two points: > 1. We should have definition of what goes or does not go into the > kernel - I think that now pretty much everything is part of the kernel > ( other the embed lib) I think there are a number of question in here: - The first one, which is just semantic, is what is the "IMP Kernel"? On reasonable answer is that it is everything defined in IMP/*.h (and internal dependencies). - The second is, what should be built into the IMP package (i.e. installed by a single install command). My answer to that is that it should include everything that is likely to be supported as long as IMP is supported. So something that is specific to a particular project and will probably not be picked up by anyone else when the implementer leaves should not be.
> 2. Daniel - can you please elaborate more on the algorithmic > differences between your "improved implementation" and the CGAL one? I > can not help wondering why a simple geometric hashing (~5 c++ classes) > can not help here.
The current grid code uses a list of grids for different sized particles and then takes each particle and searches for collisions in the other grids. Nothing is that complicated, but there are a lot of constants to be messed which with affect the speed. It would make sense to use an octtree instead of the list of grids and various other optimizations like that, but that is more work.
The CGAL code does a plane sweep through space using interval trees and reports the intersections as they arrive.
BTW, I think my attributing the nonlinearity of the grid to the radius spread was not probably not accurate. The spread should be pretty much constant in my timing code. I suspect it has more to do with the increasing number of grid cells searched, perhaps due to constants being balanced wrong or perhaps due to increased memory usage and having to use slower memory. Not sure, and I don't want to investigate now, which is my main point....
Daniel Russel wrote: > On Apr 28, 2008, at 10:14 AM, Keren Lasker wrote: > >> CGAL might be very useful for various tasks such as grid manipulation, >> motion planning, dimensionality reduction and more. >> However, I really do not think it should be part of IMP kernel , >> which should provide basic functionality common to most tasks. >> > I am not saying CGAL should be part of the kernel. I am saying that > the kernel should use CGAL (and any other external library that is > easily available and saves us work). Anything which has builds for > linux, mac os and windows covers basically all users should probably > be fair game as a dependency. Some libraries will provide core > functionality (like boost) and so IMP can't be build without them, > while others, such as CGAL currently, simply improve IMP and so are > optional. > if cgal can be installed painlessly, i.e., a normal person (non-bioinformatics person or non-computer scientist) can install it with few mouse-clicks or commands (<5) on major platforms (linux, mac, windows), i think nothing would speak against it. also the lib should not mess up the harddisk. if imp depends on libs that are hard to install it will die rather sooner than later. but otherwise, i think we all want to save time wherever possible. one possibly ignorant question: andrej got along without fancy libs. knowing him, i presume he stole the code from somebody else. can't we simply do the same? the code was pretty fast...
f > >> Two points: >> 1. We should have definition of what goes or does not go into the >> kernel - I think that now pretty much everything is part of the kernel >> ( other the embed lib) >> > I think there are a number of question in here: > - The first one, which is just semantic, is what is the "IMP Kernel"? > On reasonable answer is that it is everything defined in IMP/*.h (and > internal dependencies). > - The second is, what should be built into the IMP package (i.e. > installed by a single install command). My answer to that is that it > should include everything that is likely to be supported as long as > IMP is supported. So something that is specific to a particular > project and will probably not be picked up by anyone else when the > implementer leaves should not be. > > >> 2. Daniel - can you please elaborate more on the algorithmic >> differences between your "improved implementation" and the CGAL one? I >> can not help wondering why a simple geometric hashing (~5 c++ classes) >> can not help here. >> > > The current grid code uses a list of grids for different sized > particles and then takes each particle and searches for collisions in > the other grids. Nothing is that complicated, but there are a lot of > constants to be messed which with affect the speed. It would make > sense to use an octtree instead of the list of grids and various other > optimizations like that, but that is more work. > > The CGAL code does a plane sweep through space using interval trees > and reports the intersections as they arrive. > > BTW, I think my attributing the nonlinearity of the grid to the radius > spread was not probably not accurate. The spread should be pretty much > constant in my timing code. I suspect it has more to do with the > increasing number of grid cells searched, perhaps due to constants > being balanced wrong or perhaps due to increased memory usage and > having to use slower memory. Not sure, and I don't want to investigate > now, which is my main point.... > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev >
> if cgal can be installed painlessly, i.e., a normal person > (non-bioinformatics person or non-computer scientist) can install it > with few mouse-clicks or commands (<5) on major platforms (linux, mac, > windows), i think nothing would speak against it. also the lib should > not mess up the harddisk. I have only installed it from the command line, but that is a matter of typing "./install_cgal --prefix=where_to_install". There are binaries for windows (and I'll check on macs).
> if imp depends on libs that are hard to > install it will die rather sooner than later. Agreed. Nothing should be harder than "./configure; make install" or running a gui installer (which I think is more of a pain :-) But then again, I can't think of why any library should be harder than that. But some are.
> but otherwise, i think we > all want to save time wherever possible. > one possibly ignorant question: andrej got along without fancy libs. > knowing him, i presume he stole the code from somebody else. can't we > simply do the same? the code was pretty fast...
:-) The modeller code doesn't work because it assumes equal sized particles. He also claims he put a fair bit of effort into optimizing it. I don't really feel like spending my time that way. I have looked for good public domain code to steal, but haven't found any.
On Apr 28, 2008, at 2:58 PM, Daniel Russel wrote:
>> if cgal can be installed painlessly, i.e., a normal person >> (non-bioinformatics person or non-computer scientist) can install it >> with few mouse-clicks or commands (<5) on major platforms (linux, >> mac, >> windows), i think nothing would speak against it. also the lib should >> not mess up the harddisk. > I have only installed it from the command line, but that is a matter > of typing "./install_cgal --prefix=where_to_install". There are > binaries for windows (and I'll check on macs). probably some readme should be included somewhere in the imp distribution how to install the necessary libs if there are licensing issues in putting the whole lib into the package (containing the url, for example). > > :-) The modeller code doesn't work because it assumes equal sized > particles. He also claims he put a fair bit of effort into optimizing > it. I don't really feel like spending my time that way. I have looked > for good public domain code to steal, but haven't found any. it'd be probably a good idea to have a non-bonded list dedicated for approximately similar radii. to me it seems a general purpose non- bonded list is still too slow for most atomic calculations. from my limited experience with the old code, it seemed like you need to gain several magnitudes in computational speed to make the non-bonded list usable for atomic applications. you will be the expert on how much can be gained by improvements in the implementation and what the actual limitations of the algorithm are. probably, it will be unavoidable anyway, to have some folder designated for atomic applications anyways. e.g., there will not be a general-particle DOPE.
f
> > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
>> > probably some readme should be included somewhere in the imp > distribution how to install the necessary libs if there are licensing > issues in putting the whole lib into the package (containing the url, > for example). We definitely need instructions for the main platforms. And a decent installer and various other things.
> it'd be probably a good idea to have a non-bonded list dedicated for > approximately similar radii. to me it seems a general purpose non- > bonded list is still too slow for most atomic calculations. from my > limited experience with the old code, it seemed like you need to gain > several magnitudes in computational speed to make the non-bonded list > usable for atomic applications. If there is a speedup, we should definitely have it. The current version in IMP has a few idiocies and so is slower than need be. In addition, if things are not built with NDEBUG, they will be extremely slow as the NBL checks that is is correct each time update is called.
With the submitted code, we can relatively easily add new algorithms as we decide we want to try them and change the default algorithm as we find a better one, without forcing users to change their code.
> approximately similar radii. to me it seems a general purpose non- > bonded list is still too slow for most atomic calculations. from my > limited experience with the old code, it seemed like you need to gain > several magnitudes in computational speed to make the non-bonded list > usable for atomic applications. I think you have to build with the command "scons release=1 ..." to turn off the extra checks. Otherwise things will be slow.
>> one possibly ignorant question: andrej got along without fancy libs. >> knowing him, i presume he stole the code from somebody else. can't we >> simply do the same? the code was pretty fast... > :-) The modeller code doesn't work because it assumes equal sized > particles.
Exactly, although where we do have atomistic systems, this would still be a reasonable assumption.
> He also claims he put a fair bit of effort into optimizing it.
I think that's a little generous! It's just a standard cell-based algorithm. The only optimization I see in the code is on the hash function, and that's still a very simple function.
The Modeller code probably gets most of its performance from having optimizers that are cooperative. In order to tell whether a nonbond update is required, a simple heuristic is used: has any atom moved more than some cutoff distance since the last update? This is easy to test for because all of the optimizers update this atom shift value whenever they move an atom. In IMP there is no such guarantee - the optimizers write back new values for the optimizable attributes whenever they feel like it (plus, of course, we have attributes other than xyz).
Ben
> I think that's a little generous! It's just a standard cell-based > algorithm. The only optimization I see in the code is on the hash > function, and that's still a very simple function. By optimized, I meant tweaked grid sizes and amount particles can move and things like that. He thinks he did some work messing with it, I haven't checked :-)
> The Modeller code probably gets most of its performance from having > optimizers that are cooperative. In order to tell whether a nonbond > update is required, a simple heuristic is used: has any atom moved > more > than some cutoff distance since the last update? This is easy to test > for because all of the optimizers update this atom shift value > whenever > they move an atom. In IMP there is no such guarantee - the optimizers > write back new values for the optimizable attributes whenever they > feel > like it (plus, of course, we have attributes other than xyz). Such helps, but I don't think it is the main source of speedup. I have the MaxChangeScoreStates which play the same role (less efficiently), but it doesn't look like they are a major time sync. I can easily put in an l2 version of that (the current is l_1) which would tighten the movement bounds a bit.
One problem I was running in to when running some of my code was that the nonbonded lists would become quite huge and take up really large amounts of memory. I haven't yet found a good system for keeping the slack large enough so that we don't have to rebuild too often and not having the nbls be too big. The problem is that some things move quite far under my optimization scheme, but most things do not. I have some ideas in terms of only updating some of the pairs, but I haven't implemented them yet.
Anyway, I haven't heard back from Frido about whether he was building with NDEBUG defined or not. If not, that would be the primary problem.
Daniel Russel wrote: >> I think that's a little generous! It's just a standard cell-based >> algorithm. The only optimization I see in the code is on the hash >> function, and that's still a very simple function. >> > By optimized, I meant tweaked grid sizes and amount particles can move > and things like that. He thinks he did some work messing with it, I > haven't checked :-) > > > >> The Modeller code probably gets most of its performance from having >> optimizers that are cooperative. In order to tell whether a nonbond >> update is required, a simple heuristic is used: has any atom moved >> more >> than some cutoff distance since the last update? This is easy to test >> for because all of the optimizers update this atom shift value >> whenever >> they move an atom. In IMP there is no such guarantee - the optimizers >> write back new values for the optimizable attributes whenever they >> feel >> like it (plus, of course, we have attributes other than xyz). >> > Such helps, but I don't think it is the main source of speedup. I have > the MaxChangeScoreStates which play the same role (less efficiently), > but it doesn't look like they are a major time sync. I can easily put > in an l2 version of that (the current is l_1) which would tighten the > movement bounds a bit. > > One problem I was running in to when running some of my code was that > the nonbonded lists would become quite huge and take up really large > amounts of memory. I haven't yet found a good system for keeping the > slack large enough so that we don't have to rebuild too often and not > having the nbls be too big. The problem is that some things move quite > far under my optimization scheme, but most things do not. I have some > ideas in terms of only updating some of the pairs, but I haven't > implemented them yet. > > Anyway, I haven't heard back from Frido about whether he was building > with NDEBUG defined or not. If not, that would be the primary problem. > i have just tried - it is much faster now. plus daniel discovered a bug in my script. so the non-bonded list seems to be sufficiently fast, at least for my apps.
best
frido > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev >
Daniel Russel wrote: > - The first one, which is just semantic, is what is the "IMP Kernel"?
At the C++ level, it is whatever is linked into libimp.so. I am not fond of configure-time optional extras going into there, since it makes testing and support that much harder.
> - The second is, what should be built into the IMP package (i.e. > installed by a single install command). My answer to that is that it > should include everything that is likely to be supported as long as > IMP is supported. So something that is specific to a particular > project and will probably not be picked up by anyone else when the > implementer leaves should not be.
Indeed. All of our supported IMP modules would go into the IMP package. But we would have other IMP modules that are not supported, still in development, or are for our own use, of course, and they would be distributed separately or not at all.
Ben
On May 6, 2008, at 3:47 PM, Ben Webb wrote:
> Daniel Russel wrote: >> - The first one, which is just semantic, is what is the "IMP Kernel"? > > At the C++ level, it is whatever is linked into libimp.so. I am not > fond > of configure-time optional extras going into there, since it makes > testing and support that much harder. It is nice to avoid them, but it can simplify things and most other programs/libraries do, so I think people are used to the idea.
If we do have compile time switches, we need to add an IMP.info() method or something which prints out the build configuration. Then you can get the complete info about the IMP install used just by calling one python or C++ method without having to worry about whether various other libraries are all installed. Many users will have no clue if we ask them to see if libimpem is installed, but could give us the results of IMP.info().
>> - The second is, what should be built into the IMP package (i.e. >> installed by a single install command). My answer to that is that it >> should include everything that is likely to be supported as long as >> IMP is supported. So something that is specific to a particular >> project and will probably not be picked up by anyone else when the >> implementer leaves should not be. > > Indeed. All of our supported IMP modules would go into the IMP > package. > But we would have other IMP modules that are not supported, still in > development, or are for our own use, of course, and they would be > distributed separately or not at all.
One point that I didn't really make clear, is that I don't see that the actual library files and the build process necessarily need to follow the semantic kernel/package distinction. It may make sense to have "Packages", but still build everything into one library and one python module.
The current situation, with having to separately build and install impem is really undesirable and does not scale nicely. Also, at the moment you can't really use thing uninstalled (since the libs are scattered across many directories) and you can't easily install IMP on a mac since the paths don't get set.
Daniel Russel wrote: > One point that I didn't really make clear, is that I don't see that > the actual library files and the build process necessarily need to > follow the semantic kernel/package distinction. It may make sense to > have "Packages", but still build everything into one library and one > python module.
Sure, but that's not the model we chose for IMP; it is designed to be modular. The expectation is that most of those modules will not even be hosted in the Sali lab, let alone built into one library.
> The current situation, with having to separately build and install > impem is really undesirable and does not scale nicely.
What don't you like about that?
> Also, at the > moment you can't really use thing uninstalled (since the libs are > scattered across many directories) and you can't easily install IMP on > a mac since the paths don't get set.
You don't like 'scons install' ? That should put everything in the right place. But right now we're very far from a public release, so I don't really see the point worrying about it excessively.
Ben
> Daniel Russel wrote: >> One point that I didn't really make clear, is that I don't see that >> the actual library files and the build process necessarily need to >> follow the semantic kernel/package distinction. It may make sense to >> have "Packages", but still build everything into one library and one >> python module. > > Sure, but that's not the model we chose for IMP; it is designed to be > modular. The expectation is that most of those modules will not even > be > hosted in the Sali lab, let alone built into one library. This really has nothing to do with modularity. In either case an outside bit of code is developed and added the same way.
Things should be as simple as possible. Splitting a set of source that is maintained by the same set of people into two libraries just makes it more complicated. If the licensing is different or is the sali lab will not support a bit of code once someone graduates, then it makes sense to separate it. Or, as a much rarer case, if some block of code is so huge that it slows down linking noticeably.
>> The current situation, with having to separately build and install >> impem is really undesirable and does not scale nicely. > > What don't you like about that? There is never any reason not to build everything that can be supported by the current environment and there is no reason to make the user choose what they build explicitly. Users can choose to not use them later.
>> Also, at the >> moment you can't really use thing uninstalled (since the libs are >> scattered across many directories) and you can't easily install IMP >> on >> a mac since the paths don't get set. > > You don't like 'scons install' ? That should put everything in the > right > place. But right now we're very far from a public release, so I don't > really see the point worrying about it excessively. On a mac the horrible command scons embed=$(HOME)/src/EMbed -j $(NUMPROCS) modeller=$(MODINSTALLSVN) usecgal=1\ release=1 install prefix=$(HOME)/fs\ pyextdir=$(HOME)/fs/lib/python2.5/site-packages \ pythondir=$(HOME)/fs/lib/python2.5/site-packages impEM-install
puts things in the right place (you can't put C++ headers in /usr/ include, unless they have changed things in 10.5), but since scons doesn't make the needed calls to install_name_tool you can't link against the installed libraries. Presumably scons will fix this at some point.
My makefile which I use to wrap scons does the following to make linking work:
fix: fixem install_name_tool -id $(INSTALL)/lib/libimp.dylib $(INSTALL)/ lib/libimp.dylib install_name_tool -change kernel/src/libimp.dylib $(INSTALL)/ lib/libimp.dylib\ $(INSTALL)/lib/python2.5/site-packages/_IMP.so install_name_tool -id $(INSTALL)/lib/libimpem.dylib $ (INSTALL)/lib/libimpem.dylib install_name_tool -change kernel/src/libimp.dylib $(INSTALL)/ lib/libimp.dylib\ $(INSTALL)/lib/libimpem.dylib
Daniel Russel wrote: > Things should be as simple as possible. Splitting a set of source that > is maintained by the same set of people into two libraries just makes > it more complicated. If the licensing is different or is the sali lab > will not support a bit of code once someone graduates, then it makes > sense to separate it. Or, as a much rarer case, if some block of code > is so huge that it slows down linking noticeably.
impEM links against EMBED, which is under different licensing to IMP.
> On a mac the horrible command > scons embed=$(HOME)/src/EMbed -j $(NUMPROCS) modeller=$(MODINSTALLSVN) > usecgal=1\ > release=1 install prefix=$(HOME)/fs\ > pyextdir=$(HOME)/fs/lib/python2.5/site-packages \ > pythondir=$(HOME)/fs/lib/python2.5/site-packages impEM-install
You shouldn't need to specify pyextdir or pythondir unless something is screwed up somewhere, since they should by default live under prefix. And most of that other stuff you can just stick in config.py and never look at again.
> but since scons > doesn't make the needed calls to install_name_tool you can't link > against the installed libraries. Presumably scons will fix this at > some point.
Sure, that's easy to fix, of course.
Ben
> impEM links against EMBED, which is under different licensing to IMP. Unless it is GPL, this doesn't matter anyway. It doesn't seem to have a license currently :-)
> impEM links against EMBED, which is under different licensing to IMP. If EMBed is GPL, then impem should be a separate lib, but at least it should be built and installed along with everything else if embed is available.
Why is it under different licensing anyway?
> You shouldn't need to specify pyextdir or pythondir unless something > is > screwed up somewhere, since they should by default live under prefix. I added it since the script got the python version wrong and tried to install things in the 2.4 directory even though it ran python 2.5. Again something that will go away. At this point I've stopped using python and swig for my work and on my mac as it makes debugging too difficult and takes ages to compile.
> And most of that other stuff you can just stick in config.py and never > look at again. Most people who build it will only build once, so config.py doesn't really help them. I have a makefile, which does more or less the same thing (I have a bunch of different build commands).
Summary of my emails: - Normal people should not have to type more than scons install prefix=path [here are some extra places to look for libraries] to get absolutely everything that can be built built. - And the fewer separate pieces which can exist or not exist independently of one another, the few things we can mess up with the install script (like the problem with mac os paths in modeller I ran into a while back) and the fewer things things the user can screw up (such as by not adding -limp-somethingoranother in the right order when trying to link a C+ + program).
On Apr 26, 2008, at 1:28 PM, Ben Webb wrote:
> Daniel Russel wrote: >> On Apr 26, 2008, at 10:56 AM, Ben Webb wrote: >>> I don't think we can use CGAL. It is certainly available for Fedora, >>> but >>> we build on a lot of other platforms too. >> It is tested on quite a variety of platforms. More than IMP, but I >> have not checked if it is a superset or not. > > Indeed, but I think you missed my point: in order for us to test IMP > with CGAL on all the platforms we build on, somebody (i.e. me) has to > install CGAL on those platforms. I'm reluctant to do that unless we > really need it. What do others think? Does anybody need CGAL for > anything else? Requiring it just for a nonbonded list seems overkill. In general, building a third party library on a few platforms is way easier than supporting and optimizing a large chunk of code.
I think we should use external code as much as we possibly can and given an external library which does what we want and is widely supported, we should not hesitate to use code from it.
Daniel Russel wrote: ... > The second patches the init file as above. It also adds an argument > extraroot which adds to the include search path and the lib path (and to > the runtime link path since it is needed to make the boost tests pass > currently).
I can't apply this patch because it would break builds for everything except gcc and Python 2.5 on Linux. But I can certainly put in an equivalent feature which works everywhere if you can briefly explain the use case.
> Also, as a general point, the SConscript files > which build things should all be changed to add the local include and > lib paths to the front so that local headers mask globally installed > headers.
I'm 95% sure the include path search order is compiler-dependent, so this probably would only have the desired effect for gcc, but I agree that it's a good statement of intent if nothing else.
> The fifth adds set_particles to the the list restraint I added before > (this is independent of anything else).
Wouldn't compile for me: kernel/include/IMP/restraints/SingletonListRestraint.h:41: error: no members matching 'using IMP::Restraint::set_particles' in 'class IMP::Restraint'
As regards the nonbonded list, we cannot include CGAL in the kernel since it restricts the use of IMP to that of the QPL license. I certainly agree that CGAL has some useful features which we may want to use in IMP - but it cannot go in the kernel. Thus, it should go in an extension module. This appears to preclude use of the current patch, since Algorithm is an enum - I guess either Algorithm needs to become a class, or a CGAL_NBL class (in a module) can derive from a base NBL class (in the kernel). At the Python level it is easy to fall back from CGAL_NBL to regular NBL at runtime if the extension module is not available - just wrap the import in a try block.
Ben
>> The second patches the init file as above. It also adds an argument >> extraroot which adds to the include search path and the lib path >> (and to >> the runtime link path since it is needed to make the boost tests pass >> currently). > > I can't apply this patch because it would break builds for everything > except gcc and Python 2.5 on Linux. But I can certainly put in an > equivalent feature which works everywhere if you can briefly explain > the > use case. I didn't know how to add the required bits for other compilers (and, in fact, my solution doesn't really work for 64 bit machines).
The idea is to have a general mechanism to add a path to the compiler search paths for libraries and include files. The standard mechanisms are either to use CPPFLAGS and LIBFLAGS environment variables or add libname=path_to_lib for each libname. Ben doesn't like the former and I don't like the latter since we actually add it to the search path for all libraries. This bit me when trying to use the salilab svn modeller since imp is placed there and the compiler would mix the diva and local versions of IMP.
What I really want is an "add this path to the include search path" and "add this path to the lib search path" option that can be passed multiple times to scons so we can add more than one thing. i.e.
scons addlibpath=$HOME/fs/ib addlibpath=/opt/local/lib addincludepath= $HOME/fs/include
obviously, those names suck.
Anyone have a good idea for an interface which can be done easily in scons?
Daniel Russel wrote: > The idea is to have a general mechanism to add a path to the > compiler search paths for libraries and include files. The standard > mechanisms are either to use CPPFLAGS and LIBFLAGS environment > variables
Actually, those are for non-directory flags, e.g. debugging, optimization etc. scons uses CPPPATH and LIBPATH for the search paths.
> or add libname=path_to_lib for each libname.
This is modeled after the common autoconf-style --with-foo or --include-foo options.
> What I really want is an "add this path to the include search path" > and "add this path to the lib search path" option that can be passed > multiple times to scons so we can add more than one thing. i.e. > > scons addlibpath=$HOME/fs/ib addlibpath=/opt/local/lib addincludepath= > $HOME/fs/include
Well, you can't do that, because you can't pass arguments multiple times to scons, but as of r554 it will accept 'include' and 'lib' options to add to the include and library paths respectively. If you want to add multiple directories, you can join them with the ':' character (';' on Windows) as per usual.
Ben
> Actually, those are for non-directory flags, e.g. debugging, > optimization etc. scons uses CPPPATH and LIBPATH for the search paths. In scons yes, since it is rather more sophisticated than other systems in that regards.
> Well, you can't do that, because you can't pass arguments multiple > times > to scons, but as of r554 it will accept 'include' and 'lib' options to > add to the include and library paths respectively. If you want to add > multiple directories, you can join them with the ':' character (';' on > Windows) as per usual. BTW, apparently Options is deprecated and one is supposed to use Variables instead. There is a ListVariable which might be able to do the right thing (allow a list of validated directories), but I can't really tell from the documentation as it is exceedingly minimal.
Anyway, this is a good step. Thanks.
Daniel Russel wrote: > BTW, apparently Options is deprecated and one is supposed to use > Variables instead. There is a ListVariable which might be able to do > the right thing (allow a list of validated directories), but I can't > really tell from the documentation as it is exceedingly minimal.
No, ListVariable is the equivalent of ListOption, as far as I can tell, and that allows you to pick none, one or more values from a list. Variables also are not present in 0.96 or older scons releases, so we can't use those until we require a newer version of scons. (This won't be the newly-released 0.98 because 0.98.3 has some C++ configure test regressions, although 0.98.2 seems OK.)
Ben
>> Also, as a general point, the SConscript files >> which build things should all be changed to add the local include and >> lib paths to the front so that local headers mask globally installed >> headers. > > I'm 95% sure the include path search order is compiler-dependent, so > this probably would only have the desired effect for gcc, but I agree > that it's a good statement of intent if nothing else. I am sure they are. But given we currently only use gcc, we should at least get it working there :-)
Daniel Russel wrote: > I am sure they are. But given we currently only use gcc, we should at > least get it working there :-)
Actually the Windows builds use MSVC, of course. And various non-Linux Unixes do (or can) use other C++ compilers - for example, we use xlc on AIX and Sun C on Solaris.
Ben
On May 6, 2008, at 4:05 PM, Ben Webb wrote:
> Daniel Russel wrote: >> I am sure they are. But given we currently only use gcc, we should at >> least get it working there :-) > > Actually the Windows builds use MSVC, of course. And various non-Linux > Unixes do (or can) use other C++ compilers - for example, we use xlc > on > AIX and Sun C on Solaris. We build on lots of things, I don' t think any of us actually use IMP with anything other than gcc. The issue is one that primarily comes up when you are developing IMP or installing it multiple times, so getting it right on gcc gives us most of the current utility. That's all I way saying.
> As regards the nonbonded list, we cannot include CGAL in the kernel > since it restricts the use of IMP to that of the QPL license. I am pretty sure you are wrong on this. From my understanding QPL just says that you have to make any patches you make to the code available as patches and you can link anything you want against it. Templates are a grey area in terms of licensing since they mix linking and compilation, but we are definitely good if we have one .cpp which includes the CGAL files and that file itself is licensed under QPL. That file can be linked in with the IMP kernel (which which otherwise LGPL) without any problems.
> This appears to preclude use of the current patch, > since Algorithm is an enum - I guess either Algorithm needs to > become a > class, or a CGAL_NBL class (in a module) can derive from a base NBL > class (in the kernel). At the Python level it is easy to fall back > from > CGAL_NBL to regular NBL at runtime if the extension module is not > available - just wrap the import in a try block. It makes it quite a mess if the CGAL code is in a separate library since then either the user has to be aware of the algorithms and specifically pick to use the CGAL algorithm or we have to somehow test at runtime if the the dll can be loaded. The former is bad since most users will have no clue what the different collision detection algorithms are (and won't care) and the latter is complicated.
hi,
Frido and I are both using r->get_particles(). The function is protected, so in "our" copy we just changed IMP_LIST(protected, Particle, particle, Particle*) to public. However, now I am trying to add DOMINO to IMP - Ben could not compile it .... ;) DOMINO needs to know which particles are part of a restraint. Would you mind changing the list to public? and if not - do you have any solution ?
thank you, Keren.
You should read the emails on imp-dev occasionally :-)
Anyway, that method should definitely not be made public as it isn't guaranteed to do anything useful. For some of the restraints (NonbondedRestraints) it returns nothing and for others, such as the PairListRestraint it globs together lots of particles which aren't actually interacting.
The best solution is probably to add a new virtual method get_interacting_pairs which returns a list of interacting particle pairs (since I think that is what you want). Or something similar. It would be quite easy to implement for existing restraints.
On Aug 18, 2008, at 12:44 PM, Keren Lasker wrote:
> hi, > > Frido and I are both using r->get_particles(). > The function is protected, so in "our" copy we just changed > IMP_LIST(protected, Particle, particle, Particle*) to public. > However, now I am trying to add DOMINO to IMP - Ben could not compile > it .... ;) > DOMINO needs to know which particles are part of a restraint. > Would you mind changing the list to public? > and if not - do you have any solution ? > > thank you, > Keren. > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
thanks Daniel !
I do not really need the interacting pairs, as at the beginning of the optimization we do not really know which particles are interacting (for example - in the case of a restraint derive from MS_MS experiment). so - what I really need here is to simply get the list of particles. can we have a function like: virtual Particles* Restraint::get_particles() const;
thanks, Keren. On Aug 18, 2008, at 10:01 PM, Daniel Russel wrote:
> You should read the emails on imp-dev occasionally :-) > > Anyway, that method should definitely not be made public as it isn't > guaranteed to do anything useful. For some of the restraints > (NonbondedRestraints) it returns nothing and for others, such as the > PairListRestraint it globs together lots of particles which aren't > actually interacting. > > The best solution is probably to add a new virtual method > get_interacting_pairs which returns a list of interacting particle > pairs (since I think that is what you want). Or something similar. It > would be quite easy to implement for existing restraints. > > > On Aug 18, 2008, at 12:44 PM, Keren Lasker wrote: > >> hi, >> >> Frido and I are both using r->get_particles(). >> The function is protected, so in "our" copy we just changed >> IMP_LIST(protected, Particle, particle, Particle*) to public. >> However, now I am trying to add DOMINO to IMP - Ben could not compile >> it .... ;) >> DOMINO needs to know which particles are part of a restraint. >> Would you mind changing the list to public? >> and if not - do you have any solution ? >> >> thank you, >> Keren. >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Daniel Russel wrote: > You should read the emails on imp-dev occasionally :-)
Indeed.
> Anyway, that method should definitely not be made public as it isn't > guaranteed to do anything useful. For some of the restraints > (NonbondedRestraints) it returns nothing and for others, such as the > PairListRestraint it globs together lots of particles which aren't > actually interacting. > > The best solution is probably to add a new virtual method > get_interacting_pairs which returns a list of interacting particle > pairs (since I think that is what you want). Or something similar. It > would be quite easy to implement for existing restraints.
Agreed. I seem to recall that's what you suggested the last time Keren asked this question. I think I agreed that time around too. ;)
Keren, what is your use case? If it's not "get a list of interacting pairs" then we'll need to do something different.
Ben
On Aug 18, 2008, at 10:07 PM, Ben Webb wrote:
> Daniel Russel wrote: >> You should read the emails on imp-dev occasionally :-) > > Indeed. > >> Anyway, that method should definitely not be made public as it isn't >> guaranteed to do anything useful. For some of the restraints >> (NonbondedRestraints) it returns nothing and for others, such as the >> PairListRestraint it globs together lots of particles which aren't >> actually interacting. >> >> The best solution is probably to add a new virtual method >> get_interacting_pairs which returns a list of interacting particle >> pairs (since I think that is what you want). Or something similar. It >> would be quite easy to implement for existing restraints. > > Agreed. I seem to recall that's what you suggested the last time Keren > asked this question. I think I agreed that time around too. ;) funny !! ;) > > > Keren, what is your use case? If it's not "get a list of interacting > pairs" then we'll need to do something different. Ben - as I wrote in my last reply - it is not all_pairs, since the optimization tries to find that. We get MSMS data which says a,b,c,d,e create a complex - but we do not know which interacts with which. so what DOMINO needs out of the restraint is a-e .
> > > Ben > -- > ben@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@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
>> Keren, what is your use case? If it's not "get a list of interacting >> pairs" then we'll need to do something different. > Ben - as I wrote in my last reply - it is not all_pairs, since the > optimization tries to find that. We get MSMS data which says a,b,c,d,e > create a complex - but we do not know which interacts with which. so > what DOMINO needs out of the restraint is a-e .
It seems there are four different classes of restraints: 1) simple restraints where all particles in the restraint do interaction (for example DistanceRestraint)
2) compound restraints where there are a number of different sets of particles where the particles interactions within the set but there are no interactions between sets within the restraint: PairListRestraint or BondedListRestraint or SingletonListRestraint or most of the other restraints
3) combinatorial restraits where ultimately there will be several interacting subsets of particles, but the makeup of the subsets are not known until the end of the optimization (ConnectivityRestraint or LowestNRestraint)
4) and a group without a good name where all of the particles interact in some sense, but only a few of the interactions directly affect the final solution (such as NonBondedRestraint).
As far as I can tell, you want: 1) the set of all the particles 2) a list of sets of particles 3) the set of all the particles 4) I don't know what you want for this or perhaps just disallow it with Domino?
So a get_interacting_sets method which returns a vector of Particles's (the two s's are intentional :-) would be what you want.
On Aug 18, 2008, at 11:06 PM, Daniel Russel wrote:
>>> Keren, what is your use case? If it's not "get a list of interacting >>> pairs" then we'll need to do something different. >> Ben - as I wrote in my last reply - it is not all_pairs, since the >> optimization tries to find that. We get MSMS data which says >> a,b,c,d,e >> create a complex - but we do not know which interacts with which. so >> what DOMINO needs out of the restraint is a-e . > > It seems there are four different classes of restraints: > 1) simple restraints where all particles in the restraint do > interaction (for example DistanceRestraint) > > 2) compound restraints where there are a number of different sets of > particles where the particles interactions within the set but there > are no interactions between sets within the restraint: > PairListRestraint or BondedListRestraint or SingletonListRestraint or > most of the other restraints > > 3) combinatorial restraits where ultimately there will be several > interacting subsets of particles, but the makeup of the subsets are > not known until the end of the optimization (ConnectivityRestraint or > LowestNRestraint) > > 4) and a group without a good name where all of the particles interact > in some sense, but only a few of the interactions directly affect the > final solution (such as NonBondedRestraint). > > As far as I can tell, you want: > 1) the set of all the particles > 2) a list of sets of particles > 3) the set of all the particles > 4) I don't know what you want for this or perhaps just disallow it > with Domino? > > So a get_interacting_sets method which returns a vector of Particles's > (the two s's are intentional :-) would be what you want. > ___________
thanks Daniel ! DOMINO ignores NonBondedRestraint since otherwise the restraint graph is a clique. so yes - get_interacting_sets would be sufficient ! :) For example, for the MSMS restraint we use ConnectivityRestraint.
is your solution ok with everyone?
> ____________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
So what is the protocol for dealing with NonBondedList-like restraints? Do you feed domino a list of restraints which exclude them? That is probably better than having it try to detect and skip such restraints.
On Aug 18, 2008, at 2:15 PM, Keren Lasker wrote:
> > On Aug 18, 2008, at 11:06 PM, Daniel Russel wrote: > >>>> Keren, what is your use case? If it's not "get a list of >>>> interacting >>>> pairs" then we'll need to do something different. >>> Ben - as I wrote in my last reply - it is not all_pairs, since the >>> optimization tries to find that. We get MSMS data which says >>> a,b,c,d,e >>> create a complex - but we do not know which interacts with which. so >>> what DOMINO needs out of the restraint is a-e . >> >> It seems there are four different classes of restraints: >> 1) simple restraints where all particles in the restraint do >> interaction (for example DistanceRestraint) >> >> 2) compound restraints where there are a number of different sets of >> particles where the particles interactions within the set but there >> are no interactions between sets within the restraint: >> PairListRestraint or BondedListRestraint or SingletonListRestraint or >> most of the other restraints >> >> 3) combinatorial restraits where ultimately there will be several >> interacting subsets of particles, but the makeup of the subsets are >> not known until the end of the optimization (ConnectivityRestraint or >> LowestNRestraint) >> >> 4) and a group without a good name where all of the particles >> interact >> in some sense, but only a few of the interactions directly affect the >> final solution (such as NonBondedRestraint). >> >> As far as I can tell, you want: >> 1) the set of all the particles >> 2) a list of sets of particles >> 3) the set of all the particles >> 4) I don't know what you want for this or perhaps just disallow it >> with Domino? >> >> So a get_interacting_sets method which returns a vector of >> Particles's >> (the two s's are intentional :-) would be what you want. >> ___________ > > thanks Daniel ! > DOMINO ignores NonBondedRestraint since otherwise the restraint graph > is a clique. > so yes - get_interacting_sets would be sufficient ! :) > For example, for the MSMS restraint we use ConnectivityRestraint. > > is your solution ok with everyone? > > > > > >> ____________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
DOMINO does not know what type of restraints is given. You can potentially give a NonBondedList - but then the optimization will not be efficient. The optimization workflow ( which calls DOMINO) excludes the restraint.
On Aug 18, 2008, at 11:29 PM, Daniel Russel wrote:
> So what is the protocol for dealing with NonBondedList-like > restraints? Do you feed domino a list of restraints which exclude > them? That is probably better than having it try to detect and skip > such restraints. > > > On Aug 18, 2008, at 2:15 PM, Keren Lasker wrote: > >> >> On Aug 18, 2008, at 11:06 PM, Daniel Russel wrote: >> >>>>> Keren, what is your use case? If it's not "get a list of >>>>> interacting >>>>> pairs" then we'll need to do something different. >>>> Ben - as I wrote in my last reply - it is not all_pairs, since the >>>> optimization tries to find that. We get MSMS data which says >>>> a,b,c,d,e >>>> create a complex - but we do not know which interacts with which. >>>> so >>>> what DOMINO needs out of the restraint is a-e . >>> >>> It seems there are four different classes of restraints: >>> 1) simple restraints where all particles in the restraint do >>> interaction (for example DistanceRestraint) >>> >>> 2) compound restraints where there are a number of different sets of >>> particles where the particles interactions within the set but there >>> are no interactions between sets within the restraint: >>> PairListRestraint or BondedListRestraint or SingletonListRestraint >>> or >>> most of the other restraints >>> >>> 3) combinatorial restraits where ultimately there will be several >>> interacting subsets of particles, but the makeup of the subsets are >>> not known until the end of the optimization (ConnectivityRestraint >>> or >>> LowestNRestraint) >>> >>> 4) and a group without a good name where all of the particles >>> interact >>> in some sense, but only a few of the interactions directly affect >>> the >>> final solution (such as NonBondedRestraint). >>> >>> As far as I can tell, you want: >>> 1) the set of all the particles >>> 2) a list of sets of particles >>> 3) the set of all the particles >>> 4) I don't know what you want for this or perhaps just disallow it >>> with Domino? >>> >>> So a get_interacting_sets method which returns a vector of >>> Particles's >>> (the two s's are intentional :-) would be what you want. >>> ___________ >> >> thanks Daniel ! >> DOMINO ignores NonBondedRestraint since otherwise the restraint graph >> is a clique. >> so yes - get_interacting_sets would be sufficient ! :) >> For example, for the MSMS restraint we use ConnectivityRestraint. >> >> is your solution ok with everyone? >> >> >> >> >> >>> ____________________________________ >>> IMP-dev mailing list >>> IMP-dev@salilab.org >>> https://salilab.org/mailman/listinfo/imp-dev >> >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Restraint probably should provide an implementation of get_interacting_particles() that returns std::vector<Particles>(1, Particles(particles_begin(), particles_end()) since that is correct in many cases and just results in inefficiency if most other cases.
And I like the name "get_interacting_particles" a bit better than "get_interacting_sets" since we are talking about particles. But what do we call Particles's (i.e. std::vector<Particles>)?
On Aug 18, 2008, at 2:35 PM, Keren Lasker wrote:
> DOMINO does not know what type of restraints is given. > You can potentially give a NonBondedList - but then the optimization > will not be efficient. > The optimization workflow ( which calls DOMINO) excludes the > restraint. > > On Aug 18, 2008, at 11:29 PM, Daniel Russel wrote: > >> So what is the protocol for dealing with NonBondedList-like >> restraints? Do you feed domino a list of restraints which exclude >> them? That is probably better than having it try to detect and skip >> such restraints. >> >> >> On Aug 18, 2008, at 2:15 PM, Keren Lasker wrote: >> >>> >>> On Aug 18, 2008, at 11:06 PM, Daniel Russel wrote: >>> >>>>>> Keren, what is your use case? If it's not "get a list of >>>>>> interacting >>>>>> pairs" then we'll need to do something different. >>>>> Ben - as I wrote in my last reply - it is not all_pairs, since the >>>>> optimization tries to find that. We get MSMS data which says >>>>> a,b,c,d,e >>>>> create a complex - but we do not know which interacts with which. >>>>> so >>>>> what DOMINO needs out of the restraint is a-e . >>>> >>>> It seems there are four different classes of restraints: >>>> 1) simple restraints where all particles in the restraint do >>>> interaction (for example DistanceRestraint) >>>> >>>> 2) compound restraints where there are a number of different sets >>>> of >>>> particles where the particles interactions within the set but there >>>> are no interactions between sets within the restraint: >>>> PairListRestraint or BondedListRestraint or SingletonListRestraint >>>> or >>>> most of the other restraints >>>> >>>> 3) combinatorial restraits where ultimately there will be several >>>> interacting subsets of particles, but the makeup of the subsets are >>>> not known until the end of the optimization (ConnectivityRestraint >>>> or >>>> LowestNRestraint) >>>> >>>> 4) and a group without a good name where all of the particles >>>> interact >>>> in some sense, but only a few of the interactions directly affect >>>> the >>>> final solution (such as NonBondedRestraint). >>>> >>>> As far as I can tell, you want: >>>> 1) the set of all the particles >>>> 2) a list of sets of particles >>>> 3) the set of all the particles >>>> 4) I don't know what you want for this or perhaps just disallow it >>>> with Domino? >>>> >>>> So a get_interacting_sets method which returns a vector of >>>> Particles's >>>> (the two s's are intentional :-) would be what you want. >>>> ___________ >>> >>> thanks Daniel ! >>> DOMINO ignores NonBondedRestraint since otherwise the restraint >>> graph >>> is a clique. >>> so yes - get_interacting_sets would be sufficient ! :) >>> For example, for the MSMS restraint we use ConnectivityRestraint. >>> >>> is your solution ok with everyone? >>> >>> >>> >>> >>> >>>> ____________________________________ >>>> IMP-dev mailing list >>>> IMP-dev@salilab.org >>>> https://salilab.org/mailman/listinfo/imp-dev >>> >>> _______________________________________________ >>> IMP-dev mailing list >>> IMP-dev@salilab.org >>> https://salilab.org/mailman/listinfo/imp-dev >> >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
>> (this is independent of anything else). > > Wouldn't compile for me: > kernel/include/IMP/restraints/SingletonListRestraint.h:41: error: no > members matching 'using IMP::Restraint::set_particles' in 'class > IMP::Restraint'
I guess I skipped macros.h. Silly submit system :-)
Here you go.
> > > As regards the nonbonded list, we cannot include CGAL in the kernel > since it restricts the use of IMP to that of the QPL license. I > certainly agree that CGAL has some useful features which we may want > to > use in IMP - but it cannot go in the kernel. Thus, it should go in an > extension module. This appears to preclude use of the current patch, > since Algorithm is an enum - I guess either Algorithm needs to > become a > class, or a CGAL_NBL class (in a module) can derive from a base NBL > class (in the kernel). At the Python level it is easy to fall back > from > CGAL_NBL to regular NBL at runtime if the extension module is not > available - just wrap the import in a try block. > > Ben > -- > ben@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@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Andrej would prefer to avoid commercial-license code in IMP, and it would surely be a pain to remove this code in future if it became incompatible with our license. But he prefers more to save time now, so he's OK with linking against something like CGAL. So I can commit this patch in essentially its current form, provided that it applies against current SVN (it doesn't right now, e.g. because it's missing some files).
Ben
Since by now things are a bit mixed up, here is a monster diff.
The changes that I remember are: API Changes: - There are now just AllNonbondedListScoreState and BipartiteNonbondedListScoreState. The default algorithm should be the best currently available in your build.
- The level of checks can now be set at runtime using the methods set_check_level and get_check_level. Valid values for the levels are NONE, CHEAP, EXPENSIVE. NDEBUG works as before in that it permentantly turns off various checks but the speed difference compared to running with NONE appears to be small. I suggest that python code be run with cheap checks and interactive python run with expensive checks.
- The ChainBondedList went away as the speed advantage didn't seem to be worth having an extra class.
- A new class BondCoverScoreState is added which adds x,y,z,r to bonds passed to it in order cover the bonds with spheres for collision detection purposes.
- there is now BD optimizer. The tests for it suck and occasionally fail. I am not sure of a good way to test such stochastic things.
- the previously mentioned WLCEnergy unary function has been added
Internal API changes: - ScoreStates should now implement do_before_evaluate and do_after_evaluate instead of update and after_evaluate. Model calls before_evaluate and after_evaluate which check the iteration numbers as mentioned before and calls the internal methods if appropriate.
- I tried to roll back Ben's disabling of some tests which had been failing, but didn't try very hard, so they might not all be back.
- The default check level should probably always be cheap and the unit test should turn on expensive tests. I don't know how to add code to all IMP unit tests.
- there is code in restraints and things to return how much memory they used. I added that when I was having problems with running out of memory (due to, as it turned out, the nonbonded lists growing too large). Having something like that in our code is great. Unfortunately, I'll bet that it will slowly atrophy with time. Anyway, it is there for now as it doesn't hurt anything.
I am sure there are other changes which I forgot.
On May 12, 2008, at 3:42 PM, Ben Webb wrote:
> Andrej would prefer to avoid commercial-license code in IMP, and it > would surely be a pain to remove this code in future if it became > incompatible with our license. But he prefers more to save time now, > so > he's OK with linking against something like CGAL. So I can commit this > patch in essentially its current form, provided that it applies > against > current SVN (it doesn't right now, e.g. because it's missing some > files). > > Ben > -- > ben@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@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
could anyone also create a simple README/directory explaining, which packages (including supported versions) are needed and where to get them? the file should be be in the root IMP directory to maximize visibility. that would make the installation way simpler and avoid things like frank's hassle with boost.
thanks
frido
Daniel Russel wrote: > Since by now things are a bit mixed up, here is a monster diff. > > The changes that I remember are: > API Changes: > - There are now just AllNonbondedListScoreState and > BipartiteNonbondedListScoreState. The default algorithm should be the > best currently available in your build. > > - The level of checks can now be set at runtime using the methods > set_check_level and get_check_level. Valid values for the levels are > NONE, CHEAP, EXPENSIVE. NDEBUG works as before in that it permentantly > turns off various checks but the speed difference compared to running > with NONE appears to be small. I suggest that python code be run with > cheap checks and interactive python run with expensive checks. > > - The ChainBondedList went away as the speed advantage didn't seem to > be worth having an extra class. > > - A new class BondCoverScoreState is added which adds x,y,z,r to bonds > passed to it in order cover the bonds with spheres for collision > detection purposes. > > - there is now BD optimizer. The tests for it suck and occasionally > fail. I am not sure of a good way to test such stochastic things. > > - the previously mentioned WLCEnergy unary function has been added > > > Internal API changes: > - ScoreStates should now implement do_before_evaluate and > do_after_evaluate instead of update and after_evaluate. Model calls > before_evaluate and after_evaluate which check the iteration numbers > as mentioned before and calls the internal methods if appropriate. > > - I tried to roll back Ben's disabling of some tests which had been > failing, but didn't try very hard, so they might not all be back. > > - The default check level should probably always be cheap and the unit > test should turn on expensive tests. I don't know how to add code to > all IMP unit tests. > > - there is code in restraints and things to return how much memory > they used. I added that when I was having problems with running out of > memory (due to, as it turned out, the nonbonded lists growing too > large). Having something like that in our code is great. > Unfortunately, I'll bet that it will slowly atrophy with time. Anyway, > it is there for now as it doesn't hurt anything. > > > I am sure there are other changes which I forgot. > > > > > On May 12, 2008, at 3:42 PM, Ben Webb wrote: > >> Andrej would prefer to avoid commercial-license code in IMP, and it >> would surely be a pain to remove this code in future if it became >> incompatible with our license. But he prefers more to save time now, so >> he's OK with linking against something like CGAL. So I can commit this >> patch in essentially its current form, provided that it applies against >> current SVN (it doesn't right now, e.g. because it's missing some >> files). >> >> Ben >> -- >> ben@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@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > ------------------------------------------------------------------------ > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Friedrich Foerster wrote: > could anyone also create a simple README/directory explaining, which > packages (including supported versions) are needed and where to get > them? the file should be be in the root IMP directory to maximize > visibility.
It's in the manual: https://salilab.org/internal/manuals/imp/
Ben
It doesn't have the where to get them part (i.e. macports, fink, cygwin or whatever is easiest on windows).
As for being in the repository, we should probably have an "INSTALL" or "README" file in the repository which points to that page.
On May 13, 2008, at 1:59 PM, Ben Webb wrote:
> Friedrich Foerster wrote: >> could anyone also create a simple README/directory explaining, which >> packages (including supported versions) are needed and where to get >> them? the file should be be in the root IMP directory to maximize >> visibility. > > It's in the manual: https://salilab.org/internal/manuals/imp/ > > Ben > -- > ben@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@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Daniel Russel wrote: > It doesn't have the where to get them part (i.e. macports, fink, > cygwin or whatever is easiest on windows).
Indeed, but the manual is just a text file. Anybody can send in patches for it if they think it's missing something.
> As for being in the repository, we should probably have an "INSTALL" > or "README" file in the repository which points to that page.
Sure thing.
Ben
How do we do we build from those files? Why don't we just have all the documentation in doxygen anyway rather than have two systems? Doxygen markup is much less painful to read (although undoubtedly less powerful).
On May 13, 2008, at 2:11 PM, Ben Webb wrote:
> Daniel Russel wrote: >> It doesn't have the where to get them part (i.e. macports, fink, >> cygwin or whatever is easiest on windows). > > Indeed, but the manual is just a text file. Anybody can send in > patches > for it if they think it's missing something. > >> As for being in the repository, we should probably have an "INSTALL" >> or "README" file in the repository which points to that page. > > Sure thing. > > Ben > -- > ben@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@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Daniel Russel wrote: > How do we do we build from those files?
'scons doc'. You do, however, need to have a complete DocBook toolchain installed.
> Why don't we just have all the > documentation in doxygen anyway rather than have two systems? Doxygen > markup is much less painful to read (although undoubtedly less > powerful).
I think you answered your own question. I would hate to have to write a full manual in doxygen, although it's certainly ideal for an API reference. But natural selection will decide in the long term, if people underuse one and overuse the other.
Ben
A quick patch to make such changes is attached.
On May 13, 2008, at 1:57 PM, Friedrich Foerster wrote:
> could anyone also create a simple README/directory explaining, which > packages (including supported versions) are needed and where to get > them? the file should be be in the root IMP directory to maximize > visibility. > that would make the installation way simpler and avoid things like > frank's hassle with boost. > > thanks > > frido > > Daniel Russel wrote: >> Since by now things are a bit mixed up, here is a monster diff. >> >> The changes that I remember are: >> API Changes: >> - There are now just AllNonbondedListScoreState and >> BipartiteNonbondedListScoreState. The default algorithm should be the >> best currently available in your build. >> >> - The level of checks can now be set at runtime using the methods >> set_check_level and get_check_level. Valid values for the levels are >> NONE, CHEAP, EXPENSIVE. NDEBUG works as before in that it >> permentantly >> turns off various checks but the speed difference compared to running >> with NONE appears to be small. I suggest that python code be run with >> cheap checks and interactive python run with expensive checks. >> >> - The ChainBondedList went away as the speed advantage didn't seem to >> be worth having an extra class. >> >> - A new class BondCoverScoreState is added which adds x,y,z,r to >> bonds >> passed to it in order cover the bonds with spheres for collision >> detection purposes. >> >> - there is now BD optimizer. The tests for it suck and occasionally >> fail. I am not sure of a good way to test such stochastic things. >> >> - the previously mentioned WLCEnergy unary function has been added >> >> >> Internal API changes: >> - ScoreStates should now implement do_before_evaluate and >> do_after_evaluate instead of update and after_evaluate. Model calls >> before_evaluate and after_evaluate which check the iteration numbers >> as mentioned before and calls the internal methods if appropriate. >> >> - I tried to roll back Ben's disabling of some tests which had been >> failing, but didn't try very hard, so they might not all be back. >> >> - The default check level should probably always be cheap and the >> unit >> test should turn on expensive tests. I don't know how to add code to >> all IMP unit tests. >> >> - there is code in restraints and things to return how much memory >> they used. I added that when I was having problems with running out >> of >> memory (due to, as it turned out, the nonbonded lists growing too >> large). Having something like that in our code is great. >> Unfortunately, I'll bet that it will slowly atrophy with time. >> Anyway, >> it is there for now as it doesn't hurt anything. >> >> >> I am sure there are other changes which I forgot. >> >> >> >> >> On May 12, 2008, at 3:42 PM, Ben Webb wrote: >> >>> Andrej would prefer to avoid commercial-license code in IMP, and it >>> would surely be a pain to remove this code in future if it became >>> incompatible with our license. But he prefers more to save time >>> now, so >>> he's OK with linking against something like CGAL. So I can commit >>> this >>> patch in essentially its current form, provided that it applies >>> against >>> current SVN (it doesn't right now, e.g. because it's missing some >>> files). >>> >>> Ben >>> -- >>> ben@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@salilab.org >>> https://salilab.org/mailman/listinfo/imp-dev >> ------------------------------------------------------------------------ >> >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev > > > -- > -- > %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% > % Friedrich Foerster % > % Andrej Sali Lab % > % University of California at San Francisco % > # MC 2552 % > % Byers Hall Room 501 % > % 1700 4th Street % > % San Francisco, CA 94158-2330, USA % > % % > % phone: +1 (415) 514-4258 % > % fax: +1 (415) 514-4231 % > % % > % www.salilab.org/~frido % > % % > %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
Daniel Russel wrote: > Since by now things are a bit mixed up, here is a monster diff.
Most of this should be in now. If I missed anything, send in a (hopefully less monstrous) diff...
Although everything compiles, MSVC spits out a bunch of warnings about the MKSUnit stuff; perhaps you could take a look and either satisfy yourself that this isn't a problem or (ideally) disambiguate things sufficiently so that it's happy: https://salilab.org/internal/imp/logs/imp/bin.i386-w32.log
> - The level of checks can now be set at runtime using the methods > set_check_level and get_check_level. Valid values for the levels are > NONE, CHEAP, EXPENSIVE. NDEBUG works as before in that it permentantly > turns off various checks but the speed difference compared to running > with NONE appears to be small. I suggest that python code be run with > cheap checks and interactive python run with expensive checks.
I'll put this in shortly. Maybe I'm mistaken but it looked like NDEBUG didn't permanently disable the checks, just set the default check level. Most users would expect NDEBUG to disable the stuff at compile time. I'll check this to make sure that's really the case before it goes in.
> - A new class BondCoverScoreState is added > > - there is now BD optimizer.
I didn't see any testcases for these in the patch...
> I am not sure of a good way to test such stochastic things.
Usually you can only really test average properties, e.g. energy conservation.
> - I tried to roll back Ben's disabling of some tests which had been > failing, but didn't try very hard, so they might not all be back.
Why would you want to do that? Those were bug fixes. The tests weren't failing - they were exposing bugs. By rolling back the changes, you reintroduce the bugs...
> - The default check level should probably always be cheap and the unit > test should turn on expensive tests. I don't know how to add code to all > IMP unit tests.
Put it in the base class's setUp() method, I guess. You just need to be a little bit careful that that is called by any subclass that also defines a setUp method. But I can do that.
> - there is code in restraints and things to return how much memory they > used.
I don't like this so much, since you add a virtual method to Object, which increases the overhead for everything. If you have to instrument, couldn't you just instrument the Restraint and/or ScoreState classes?
Ben
> Most of this should be in now. If I missed anything, send in a > (hopefully less monstrous) diff... > I'll send in the little bits I or you missed. Thanks.
> Although everything compiles, MSVC spits out a bunch of warnings about > the MKSUnit stuff; perhaps you could take a look and either satisfy > yourself that this isn't a problem or (ideally) disambiguate things > sufficiently so that it's happy: > https://salilab.org/internal/imp/logs/imp/bin.i386-w32.log > I looked this morning and, as far as I could tell, it was warning about using a standard feature of C++ that I was intentionally using. MSVC has ways to turning off specific warnings, so I may go and turn the warnings off for those calls.
> I'll put this in shortly. Maybe I'm mistaken but it looked like NDEBUG > didn't permanently disable the checks, just set the default check level. > Most users would expect NDEBUG to disable the stuff at compile time. > I'll check this to make sure that's really the case before it goes in. > The patch is a bit hazy on what NDEBUG should do (reflecting my haziness). In general, NDEBUG should do or not do whatever we want it to do (and turning it on shouldn't make code slower). Other than that, I wouldn't worry too much about user expectations, especially since we don't allow the users to control it directly.
> > > Why would you want to do that? Those were bug fixes. The tests weren't > failing - they were exposing bugs. By rolling back the changes, you > reintroduce the bugs... > From what I understand, the tests were exposing bugs in IMP. The patches probably should have fixed the bugs and in case we should fix the bugs in IMP rather than just disable the tests. Or am I confused.
> - there is code in restraints and things to return how much memory they > used. > > > I don't like this so much, since you add a virtual method to Object, > which increases the overhead for everything. If you have to instrument, > couldn't you just instrument the Restraint and/or ScoreState classes? > Instrumenting the other base classes is probably sufficient. Anyway, the code is only occasionally useful, annoying to keep up to date and pretty much impossible to check meaningfully, which means it probably will end up incorrect. So dropping it is fine with me.
Daniel Russel wrote: >> Although everything compiles, MSVC spits out a bunch of warnings about >> the MKSUnit stuff > I looked this morning and, as far as I could tell, it was warning about > using a standard feature of C++ that I was intentionally using. MSVC has > ways to turning off specific warnings, so I may go and turn the warnings > off for those calls.
Good; that was my understanding too. It's unfortunate that MSVC sucks.
> The patch is a bit hazy on what NDEBUG should do (reflecting my > haziness). In general, NDEBUG should do or not do whatever we want it to > do (and turning it on shouldn't make code slower). Other than that, I > wouldn't worry too much about user expectations, especially since we > don't allow the users to control it directly.
The only thing we can say for sure is that when NDEBUG is defined, assert() is a noop. IMHO, IMP_assert() acts like assert(), so it should do the same thing - that's how I put it in the patch I committed. For the other runtime checks, it seems reasonable for it to be like CHEAP or NONE, as you say.
> From what I understand, the tests were exposing bugs in IMP. The > patches probably should have fixed the bugs and in case we should fix > the bugs in IMP rather than just disable the tests. Or am I confused.
You are probably confused because it did both. ;) The patch did fix the bug in IMP (use of invalidated iterators). It also disabled the particle deletion part of the tests, because IMP was touching freed particle pointers in that case. Arguably that's also a bug in IMP, but it was my understanding that we can't "fix" that without putting in particle reference counting, which I didn't have the time to do.
Ben
> > The only thing we can say for sure is that when NDEBUG is defined, > assert() is a noop. IMHO, IMP_assert() acts like assert(), so it > should do the same thing - that's how I put it in the patch I > committed. For the other runtime checks, it seems reasonable for it to > be like CHEAP or NONE, as you say. I don't think we need to provide that guarantee (perhaps we should rename the macros to IMP_internal_correctness_check since they are checks which should not fail if there are no bugs in IMP).
Part of my goal was to get rid of having two different types of compilation which we need to pay attention to.
participants (5)
-
Ben Webb
-
Daniel Russel
-
Daniel Russel
-
Friedrich Foerster
-
Keren Lasker