- IMP-dev - salilab.org

More small patches
by Daniel Russel 16 May '08

16 May '08

- Modify MaxChangeScoreState to use the list macros. - Clean up various attempts at automatically picking parameters in the nonbonded lists. For now just have them be user setable. Also, I don't think that functions which are not part of the API should be given doxygen comments. Index: kernel/include/IMP/score_states/MaxChangeScoreState.h =================================================================== --- kernel/include/IMP/score_states/MaxChangeScoreState.h (revision 560) +++ kernel/… [View More]include/IMP/score_states/MaxChangeScoreState.h (working copy) @@ -27,9 +27,7 @@ { FloatKeys keys_; FloatKeys origkeys_; - Particles ps_; float max_change_; - virtual void do_before_evaluate(); public: //! Track the changes with the specified keys. MaxChangeScoreState(const FloatKeys &keys, @@ -37,6 +35,8 @@ virtual ~MaxChangeScoreState(){} + IMP_SCORE_STATE(internal::kernel_version_info); + //! Measure differences from the current value. void reset(); Index: kernel/src/score_states/MaxChangeScoreState.cpp =================================================================== --- kernel/src/score_states/MaxChangeScoreState.cpp (revision 560) +++ kernel/src/score_states/MaxChangeScoreState.cpp (working copy) @@ -74,4 +74,8 @@ max_change_=0; } +void MaxChangeScoreState::show(std::ostream &out) const { + out << "MaxChangeScoreState" << std::endl; +} + } // namespace IMP Index: kernel/include/IMP/score_states/NonbondedListScoreState.h =================================================================== --- kernel/include/IMP/score_states/NonbondedListScoreState.h (revision 560) +++ kernel/include/IMP/score_states/NonbondedListScoreState.h (working copy) @@ -42,15 +42,19 @@ /** How much to add to the size of particles to allow particles to move without rebuilding the list */ Float slack_; - //! An estimate of what the slack should be next time the list is recomputed - Float next_slack_; - int num_steps_; bool nbl_is_valid_; int number_of_rebuilds_; int number_of_updates_; + int number_of_overflows_; + //! The maximum allowable size for the NBL + /** An exception will be thrown if the list exceeds this size. + */ + unsigned int max_nbl_size_; typedef std::vector<std::pair<Particle*, Particle*> > NBL; NBL nbl_; + struct NBLTooLargeException{}; + protected: Float get_slack() const {return slack_;} @@ -93,7 +97,11 @@ if (!are_bonded(a,b)) { IMP_LOG(VERBOSE, "Found pair " << a->get_index() << " " << b->get_index() << std::endl); - nbl_.push_back(std::make_pair(a, b)); + if (nbl_.size() < max_nbl_size_) { + nbl_.push_back(std::make_pair(a, b)); + } else { + throw NBLTooLargeException(); + } } else { IMP_LOG(VERBOSE, "Pair " << a->get_index() << " and " << b->get_index() << " rejected on bond" @@ -184,6 +192,29 @@ FloatKey get_radius_key() const {return rk_;} void set_radius_key(FloatKey rk) {rk_=rk;} + //! Set the maximum allowable size for the NBL + /** The NBL will keep reducing the slack and trying to + rebuild until it can make the list smaller than this. + */ + void set_max_size(unsigned int mx) { + max_nbl_size_= mx; + } + + + //! Set the slack used when generating the NBL + /** The slack allows the the NBL to non be rebuilt every step + making the process more efficient. However, too large + a value can result in the NBL being excessively large. + + A good guideline is that it should be the maximum amount + a particle coordinate would change in 20 steps or so. + */ + void set_slack(float slack) { + IMP_check(slack>= 0, "Slack must be nonnegative", + ValueException("Negative slack")); + slack_=slack; + } + IMP_CONTAINER(BondedListScoreState, bonded_list, BondedListIndex); Index: kernel/src/score_states/NonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/NonbondedListScoreState.cpp (revision 560) +++ kernel/src/score_states/NonbondedListScoreState.cpp (working copy) @@ -37,11 +37,11 @@ cutoff_(cut), nbl_is_valid_(false) { - slack_=20; - next_slack_=slack_; - num_steps_=1; + slack_=cutoff_; number_of_updates_=1; number_of_rebuilds_=0; + number_of_overflows_=0; + max_nbl_size_= std::numeric_limits<unsigned int>::max(); } @@ -55,7 +55,9 @@ { out << "Nonbonded list averaged " << static_cast<Float>(number_of_updates_) - / number_of_rebuilds_ << " steps between rebuilds" << std::endl; + / number_of_rebuilds_ << " steps between rebuilds" + << " and overflowed " << number_of_overflows_ + << " times" << std::endl; } @@ -96,47 +98,25 @@ (*bli)->before_evaluate(ScoreState::get_before_evaluate_iteration()); } - if (nbl_is_valid_) { - /*std::cout << "Rate is " << rate << " target is " << target_steps - << " so slack is " << target_slack << " mc " << mc - << " nbl " << nbl_.size() << " cost " - << rebuild_cost << std::endl;*/ - if (mc > slack_) { - /* Float rate= std::pow(static_cast<Float>(nbl_.size()), - .333f)/ num_steps_; - Float target_steps= .6*std::pow(rebuild_cost, .25f) - *std::pow(rate, -.75f); - Float target_slack= (target_steps+1)*mc/num_steps_; - next_slack_= target_slack*.5 + .5*next_slack_; - */ - - /*std::cout << "Killing nbl because " << mc << " - << slack_ << " " << next_slack_ - << " " << num_steps_ << std::endl;*/ - if (num_steps_ < 50) { - //slack_= next_slack_; - } - num_steps_=1; - //next_slack_= std::max(2.0*mc, 2.0*slack_); - set_nbl_is_valid(false); - ++number_of_rebuilds_; - slack_=next_slack_; - /*} else if (num_steps_ > 100) { - //next_slack_=slack_/2.0; - slack_=next_slack_; - num_steps_=1; - set_nbl_is_valid(false); - ++number_of_rebuilds_;*/ - } else { - ++num_steps_; - //next_slack_= next_slack_*.98; - } - } - bool rebuilt=false; if (!get_nbl_is_valid()) { - rebuild_nbl(); - rebuilt=true; + unsigned int rebuild_attempts=0; + do { + try { + ++rebuild_attempts; + ++number_of_rebuilds_; + rebuild_nbl(); + rebuilt=true; + } catch (NBLTooLargeException &) { + slack_= slack_/2.0; + ++number_of_overflows_; + if (number_of_rebuilds_==100) { + IMP_WARN("Can't rebuild NBL with given max NBL size of " + << max_nbl_size_ << std::endl); + throw ValueException("Bad NBL max size"); + } + } + } while (!rebuilt); } else { nbl_.erase(std::remove_if(nbl_.begin(), nbl_.end(), internal::HasInactive()), [View Less]

2 3

Minor patches
by Daniel Russel 16 May '08

16 May '08

First, Harmonic.h needs to include utility.h since it uses square. Second, I changed the check macro to be IMP_IF_CHECK as discussed. The patch also brings in a few other cleanups to the runtime checks. Index: kernel/include/IMP/unary_functions/Harmonic.h =================================================================== --- kernel/include/IMP/unary_functions/Harmonic.h (revision 557) +++ kernel/include/IMP/unary_functions/Harmonic.h (working copy) @@ -8,6 +8,7 @@ #define __IMP_HARMONIC_H … [View More] #include "../UnaryFunction.h" +#include "../utility.h" namespace IMP { Index: kernel/include/IMP/internal/ObjectContainer.h =================================================================== --- kernel/include/IMP/internal/ObjectContainer.h (revision 557) +++ kernel/include/IMP/internal/ObjectContainer.h (working copy) @@ -8,6 +8,8 @@ #ifndef __IMP_OBJECT_CONTAINER_H #define __IMP_OBJECT_CONTAINER_H +#include "Object.h" + #include <boost/iterator/filter_iterator.hpp> #include <vector> @@ -104,15 +106,15 @@ return Vector::operator[](get_index(i)); } I push_back(O* d) { + IMP_CHECK_OBJECT(d); ref(d); - IMP_BEGIN_CHECK(EXPENSIVE); - IMP_CHECK_OBJECT(d); - for (typename Vector::const_iterator it= Vector::begin(); - it != Vector::end(); ++it) { - IMP_assert(*it != d, "IMP Containers can only have one copy of " - << " each object"); + IMP_IF_CHECK(EXPENSIVE) { + for (typename Vector::const_iterator it= Vector::begin(); + it != Vector::end(); ++it) { + IMP_assert(*it != d, "IMP Containers can only have one copy of " + << " each object"); + } } - IMP_END_CHECK; if (free_.empty()) { Vector::push_back(d); unsigned int idx= Vector::size()-1; @@ -126,16 +128,16 @@ } template <class It> void insert(iterator c, It b, It e) { - IMP_BEGIN_CHECK(EXPENSIVE); - for (It cc= b; cc != e; ++cc) { - IMP_CHECK_OBJECT(*cc); - for (typename Vector::const_iterator it= Vector::begin(); - it != Vector::end(); ++it) { - IMP_assert(*it != *cc, "IMP Containers can only have one copy of " - << " each object"); + IMP_IF_CHECK(EXPENSIVE) { + for (It cc= b; cc != e; ++cc) { + IMP_CHECK_OBJECT(*cc); + for (typename Vector::const_iterator it= Vector::begin(); + it != Vector::end(); ++it) { + IMP_assert(*it != *cc, "IMP Containers can only have one copy of " + << " each object"); + } } } - IMP_END_CHECK; for (It cc= b; cc != e; ++cc) { ref(*cc); } Index: kernel/include/IMP/exception.h =================================================================== --- kernel/include/IMP/exception.h (revision 557) +++ kernel/include/IMP/exception.h (working copy) @@ -125,12 +125,10 @@ IMPDLLEXPORT CheckLevel get_check_level(); -#define IMP_BEGIN_CHECK(level)\ - if (level <= ::IMP::get_check_level()) { do {} while(false) -#define IMP_END_CHECK } do {} while(false) +#define IMP_IF_CHECK(level)\ + if (level <= ::IMP::get_check_level()) - namespace internal { Index: kernel/src/Model.cpp =================================================================== --- kernel/src/Model.cpp (revision 557) +++ kernel/src/Model.cpp (working copy) @@ -101,6 +101,20 @@ IMP_LOG(TERSE, "End Model::evaluate. Final score: " << score << std::endl); + for (ParticleIterator pit= particles_begin(); + pit != particles_end(); ++pit) { + for (Particle::FloatKeyIterator fkit = (*pit)->float_keys_begin(); + fkit != (*pit)->float_keys_end(); ++fkit) { + Float v= (*pit)->get_value(*fkit); + IMP_check(v==v, "NaN found in particle " << (*pit)->get_index() + << " attribute " << *fkit, + ValueException("NaN found")); + Float d= (*pit)->get_derivative(*fkit); + IMP_check(d==d, "NaN found in particle derivative " << (*pit)->get_index() + << " attribute " << *fkit, + ValueException("NaN found")); + } + } ++iteration_; return score; } Index: kernel/src/Particle.cpp =================================================================== --- kernel/src/Particle.cpp (revision 557) +++ kernel/src/Particle.cpp (working copy) @@ -25,10 +25,14 @@ void Particle::set_model(Model *md, ParticleIndex pi) { + IMP_check(model_==NULL || md==NULL, + "Set_model called for particle already in model", + ValueException("Cannot transfer particles directly")); model_ = md; pi_ = pi; - IMP_assert(model_==NULL || model_->get_particle(pi_)== this, - "Set_model called with inconsistent data"); + IMP_check(model_==NULL || model_->get_particle(pi_)== this, + "Set_model called with inconsistent data", + ValueException("Cannot transfter particles directly")); } void Particle::set_is_active(const bool is_active) Index: kernel/src/score_states/BondDecoratorListScoreState.cpp =================================================================== --- kernel/src/score_states/BondDecoratorListScoreState.cpp (revision 557) +++ 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/BipartiteNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/BipartiteNonbondedListScoreState.cpp (revision 557) +++ kernel/src/score_states/BipartiteNonbondedListScoreState.cpp (working copy) @@ -17,7 +17,7 @@ //! Turn the default into an actual algorithm and work around missing algorithms /** This cannot be shared with AllNBL because that one has grid and this does - not. + not. */ static BipartiteNonbondedListScoreState::Algorithm translate_algorithm(BipartiteNonbondedListScoreState::Algorithm a) @@ -100,9 +100,9 @@ mc1_->reset(); mcr_->reset(); } - IMP_BEGIN_CHECK(EXPENSIVE); - check_nbl(); - IMP_END_CHECK; + IMP_IF_CHECK(EXPENSIVE) { + check_nbl(); + } } void BipartiteNonbondedListScoreState::process_sets(const Particles &p0, Index: kernel/src/score_states/AllNonbondedListScoreState.cpp =================================================================== --- kernel/src/score_states/AllNonbondedListScoreState.cpp (revision 557) +++ kernel/src/score_states/AllNonbondedListScoreState.cpp (working copy) @@ -95,9 +95,9 @@ if (P::update(mc, cost)) { mc_->reset(); } - IMP_BEGIN_CHECK(EXPENSIVE); - check_nbl(); - IMP_END_CHECK; + IMP_IF_CHECK(EXPENSIVE) { + check_nbl(); + } } Index: kernel/src/pair_scores/DistancePairScore.cpp =================================================================== --- kernel/src/pair_scores/DistancePairScore.cpp (revision 557) +++ kernel/src/pair_scores/DistancePairScore.cpp (working copy) @@ -29,14 +29,13 @@ Float d2 = 0, delta[3]; Float score; - IMP_BEGIN_CHECK(CHEAP); - XYZDecorator::cast(a); - XYZDecorator::cast(b); - IMP_END_CHECK; + IMP_IF_CHECK(CHEAP) { + XYZDecorator::cast(a); + XYZDecorator::cast(b); + } XYZDecorator d0(a); XYZDecorator d1(b); - for (int i = 0; i < 3; ++i) { delta[i] = d0.get_coordinate(i) - d1.get_coordinate(i); d2 += square(delta[i]); Index: kernel/src/pair_scores/SphereDistancePairScore.cpp =================================================================== --- kernel/src/pair_scores/SphereDistancePairScore.cpp (revision 557) +++ kernel/src/pair_scores/SphereDistancePairScore.cpp (working copy) @@ -21,6 +21,12 @@ Float SphereDistancePairScore::evaluate(Particle *a, Particle *b, DerivativeAccumulator *da) { + IMP_check(a->has_attribute(radius_), "Particle " << a->get_index() + << "missing radius in SphereDistancePairScore", + ValueException("Missing radius")); + IMP_check(b->has_attribute(radius_), "Particle " << b->get_index() + << "missing radius in SphereDistancePairScore", + ValueException("Missing radius")); Float ra = a->get_value(radius_); Float rb = b->get_value(radius_); return internal::evaluate_distance_pair_score(a,b, da, f_.get(), Index: kernel/src/internal/bbox_nbl_helpers.cpp =================================================================== --- kernel/src/internal/bbox_nbl_helpers.cpp (revision 557) +++ kernel/src/internal/bbox_nbl_helpers.cpp (working copy) @@ -9,6 +9,10 @@ #include "IMP/decorators/XYZDecorator.h" #include "IMP/score_states/NonbondedListScoreState.h" +/* compile the CGAL code with NDEBUG since it doesn't have the + same level of control over errors as IMP +*/ +#define NDEBUG #ifdef IMP_USE_CGAL #include <CGAL/box_intersection_d.h> #include <vector> [View Less]

1 0

IMP_BEGIN_CHECK
by Daniel Russel 15 May '08

15 May '08

As any of you who looked at the syntax may have noticed, the current syntax is a bit evil since you write IMP_BEGIN_CHECK(EXPENSIVE); some stuff which gets controlled by the surrounding statement and is in a nested scope IMP_END_CHECK; We want something that naturally conveys that the surrounded blocks are in a nested scope. One possibility is something like IMP_IF_CHECK(level) which would expand to if (get_check_level() > level) So the user has to add braces to put more than one … [View More]

2 1

ObjectContainer disabling checks
by Daniel Russel 15 May '08

15 May '08

I had missed wrapping one of the checks in ObjectContainer with NDEBUG which made adding large numbers of particles painfully slow. I can't submit a patch since it would conflict with the not yet committed changes. It is simple, the single object insert needs to have its check wrapped with IMP_BEGIN_CHECK(EXPENSIVE); IMP_END_CHECK And the multipartcle check which is currently CHEAP should be made EXPENSIVE.

1 0

GravityCenter::set_position...
by Daniel Russel 13 May '08

13 May '08

really shouldn't be called that as we have set_X take a value everywhere else. The method updates the position from its parts and so should be called something more like that.

1 0

Re: [IMP-dev] NBL cleanup and other small patches
by Ben Webb 13 May '08

13 May '08

Daniel Russel wrote: >> 'scons doc'. You do, however, need to have a complete DocBook toolchain >> installed. > Well, I guess flute doesn't have such then (although it does seem to > have some of docbook installed) > scons doc > blah, blah, blah > scons *** Do not know how to make target 'doc'. Stop. Oh, sorry. It should be 'kernel/doc'. Forgot that we moved all the kernel stuff to a subdirectory some time ago. > I haven't used docbook. For what we have so far, … [View More]

2 2

minor patch
by Friedrich Foerster 12 May '08

12 May '08

would anybody object the below patch? we (keren and me) need it for magic sphimp (i don't remember why, but keren will know). thanks frido Index: kernel/include/IMP/Restraint.h =================================================================== --- kernel/include/IMP/Restraint.h (revision 539) +++ kernel/include/IMP/Restraint.h (working copy) @@ -94,7 +94,8 @@ return model_.get(); } - IMP_LIST(protected, Particle, particle, Particle*) + //IMP_LIST(protected, Particle, … [View More]

3 3

Ordering among score_states (and other things)
by Daniel Russel 08 May '08

08 May '08

I am ending up with various score states which have to be updated in the right order. Currently this requires that the user add them to the model in the right order and non-obvious things happen if this is not the case. I think adding an iteration counter to the update call would make this simpler (and the change could be implemented without changing individual score states much). Model maintains a counter which is incremented each time evaluate is called. It then calls ScoreState:… [View More]

2 3

Cleanup of attribute table
by Daniel Russel 07 May '08

07 May '08

I cleaned up and simplified the attribute table handing. You can now declare keys types anywhere (although the injunction against static initialization of keys is still there). And there is some explanation of what is going on.

1 0

Remove unused python class in test
by Daniel Russel 07 May '08

07 May '08

I removed an unused class from the test case

1 0