- IMP-dev - salilab.org

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 #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>

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 statement in there. I think I like that better and emacs does the right thing with indenting it. Any thoughts?

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, doxygen is much simpler > (and prettier :-), but perhaps you have some more involved directions to > take things. Docbook certain went overboard on the markup :-) Indeed - it's XML. The DocBook stuff right now is very basic, because most of today's documentation is API documentation. But once we have less technical users - as is the long term intention - they will need more traditional book-style documentation (of course pointing them to the docbook docs for any technical stuff). Ben -- ben(a)salilab.org http://salilab.org/~ben/ "It is a capital mistake to theorize before one has data." - Sir Arthur Conan Doyle

2 2

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, particle, Particle*) + IMP_LIST(public, Particle, particle, Particle*) private: internal::ObjectPointer<Model, false> model_;

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::update(int) for each score state passing the counter. This update is a method in the ScoreState base class which simply compares the passed counter to the stored counter and calls the existing update method if the counters are different. ScoreStates which depend on other score states can simply call update on their dependencies with the counter. Each ScoreState is updated exactly once and, unless there are circular dependencies, everything is updated before it is used. The same thing can be used for after_evaluate, although there is currently no need. With slightly more logic we could catch circular dependencies too. Comments?

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

Limiting the size of moves with conjugate gradients
by Daniel Russel 07 May '08

07 May '08

In an attempt to decrease the frequency with which CG blew up due to stepping to far and getting floating point overflow, I added a parameter which restricts the maximum change for any attribute. It helps is my code, but is a bit of a hack.

1 0