- IMP-dev - salilab.org

Actual patches
by Daniel Russel 28 May '08

28 May '08

First, the patch fixes a bug in remove_always and removes the workaround in Particle. The second fixes a bug in reference counting and removes the workaround in the test code. In addition, for some reason all the reference counting tests were disabled so that fact that they all were failing didn't come up :-) Anyway, now things work and are tested. The containers now should deal with exceptions better (for example adding a non-reference counted Object to two containers and catching the resulting exception doesn't result in memory corruption any more). Not that the rest of the IMP code is all exception safe, but these changes should handle most errors. Index: kernel/include/IMP/internal/AttributeTable.h =================================================================== --- kernel/include/IMP/internal/AttributeTable.h (revision 584) +++ kernel/include/IMP/internal/AttributeTable.h (working copy) @@ -199,7 +199,9 @@ } void remove_always(Key k) { - map_[k.get_index()]= Traits::get_invalid(); + if (k.get_index() < size_) { + map_[k.get_index()]= Traits::get_invalid(); + } } Index: kernel/include/IMP/Particle.h =================================================================== --- kernel/include/IMP/Particle.h (revision 584) +++ kernel/include/IMP/Particle.h (working copy) @@ -265,6 +265,9 @@ \return true it the particle is active. */ bool get_is_active() const { + IMP_IF_CHECK(EXPENSIVE) { + assert_is_valid(); + } return is_active_; } @@ -543,9 +546,7 @@ { floats_.remove(name); derivatives_.remove(name); - if (optimizeds_.contains(name)) { - optimizeds_.remove_always(name); - } + optimizeds_.remove_always(name); } Index: kernel/include/IMP/internal/ObjectContainer.h =================================================================== --- kernel/include/IMP/internal/ObjectContainer.h (revision 584) +++ kernel/include/IMP/internal/ObjectContainer.h (working copy) @@ -9,6 +9,7 @@ #define __IMP_OBJECT_CONTAINER_H #include "Object.h" +#include "RefCountedObject.h" #include <boost/iterator/filter_iterator.hpp> @@ -31,8 +32,9 @@ couldn't get the casts to work out right. */ template <class O, class I> -class ObjectContainer: public std::vector<O*> +class ObjectContainer { + std::vector<O*> data_; std::vector<int> free_; struct OK { bool operator()(const O*a) const { @@ -44,108 +46,111 @@ unsigned int get_index(II i) const {return i.get_index();} unsigned int get_index(unsigned int i) const {return i;} - void ref(O*o) { - if (o) o->ref(); - } - - void unref(O* o) { - if (o) { - o->unref(); - if (!o->get_has_ref()) { - delete o; - } - } - } // hide it - void erase(){} + typedef std::vector<O*> Vector; public: - typedef std::vector<O*> Vector; - using Vector::size; - using Vector::empty; + ObjectContainer(){} ~ObjectContainer() { clear(); } + bool empty() const { + return data_.empty() || data_.size() == free_.size(); + } + unsigned int size() const { + return data_.size() - free_.size(); + } + void clear() { - for (typename Vector::iterator it= Vector::begin(); - it != Vector::end(); ++it) { - unref(*it); + for (typename Vector::iterator it= data_.begin(); + it != data_.end(); ++it) { + O* t= *it; + *it=NULL; + if (t) disown(t); } free_.clear(); - Vector::clear(); + data_.clear(); } typedef boost::filter_iterator<OK, typename Vector::iterator> iterator; - iterator begin() {return iterator(Vector::begin(), Vector::end());} - iterator end() {return iterator(Vector::end(), Vector::end());} + iterator begin() {return iterator(data_.begin(), data_.end());} + iterator end() {return iterator(data_.end(), data_.end());} typedef boost::filter_iterator<OK, typename Vector::const_iterator> const_iterator; const_iterator begin() const { - return const_iterator(Vector::begin(), Vector::end()); + return const_iterator(data_.begin(), data_.end()); } const_iterator end() const { - return const_iterator(Vector::end(), Vector::end()); + return const_iterator(data_.end(), data_.end()); } void remove(I i) { unsigned int id= get_index(i); - IMP_assert(Vector::operator[](id) != NULL, "Nothing there to remove"); - unref(Vector::operator[](id)); - Vector::operator[](id)=NULL; + IMP_assert(id < data_.size(), + "Trying to remove invalid element in container"); + IMP_assert(data_[id] != NULL, "Nothing there to remove"); + O* t= data_[id]; free_.push_back(id); + data_[id]=NULL; + disown(t); } O* operator[](I i) const { - IMP_check(get_index(i) < Vector::size(), + IMP_check(get_index(i) < data_.size(), "Index " << i << " out of range", IndexException("Out of range")); - IMP_assert(Vector::operator[](get_index(i)) != NULL, + IMP_assert(data_.operator[](get_index(i)) != NULL, "Attempting to access invalid slot in container"); - return Vector::operator[](get_index(i)); + return data_[get_index(i)]; } + I push_back(O* d) { IMP_CHECK_OBJECT(d); - ref(d); + own(d); IMP_IF_CHECK(EXPENSIVE) { - for (typename Vector::const_iterator it= Vector::begin(); - it != Vector::end(); ++it) { + for (typename Vector::const_iterator it= data_.begin(); + it != data_.end(); ++it) { IMP_assert(*it != d, "IMP Containers can only have one copy of " << " each object"); } } if (free_.empty()) { - Vector::push_back(d); - unsigned int idx= Vector::size()-1; + data_.push_back(d); + unsigned int idx= data_.size()-1; return I(idx); } else { unsigned int i= free_.back(); free_.pop_back(); - Vector::operator[](i)= d; + data_[i]= d; return I(i); } } + template <class It> void insert(iterator c, It b, It e) { + IMP_assert(c== end(), "Insert position is ignored in ObjectContainer"); 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) { + for (typename Vector::const_iterator it= data_.begin(); + it != data_.end(); ++it) { IMP_assert(*it != *cc, "IMP Containers can only have one copy of " << " each object"); } } } for (It cc= b; cc != e; ++cc) { - ref(*cc); + own(*cc); } while (!free_.empty()) { - push_back(*b); + int i= free_.back(); + free_.pop_back(); + data_[i]= *b; ++b; } - Vector::insert(c.base(), b, e); + data_.insert(data_.end(), b, e); } }; Index: kernel/include/IMP/internal/Object.h =================================================================== --- kernel/include/IMP/internal/Object.h (revision 584) +++ kernel/include/IMP/internal/Object.h (working copy) @@ -9,7 +9,6 @@ #ifndef __IMP_OBJECT_H #define __IMP_OBJECT_H -#include "../log.h" #include "../exception.h" namespace IMP @@ -42,24 +41,27 @@ void assert_is_valid() const; bool get_has_ref() const {return count_ != 0;} + void ref() { assert_is_valid(); - IMP_assert(count_== 0, - "Non-reference counted objects can only belong to " \ - "one container."); ++count_; } + void unref() { assert_is_valid(); - IMP_assert(count_ ==1, "Too many unrefs on object"); + IMP_assert(count_ !=0, "Too many unrefs on object"); --count_; } - protected: - int count_; -private: + + unsigned int get_ref_count() const { + return count_; + } + + private: Object(const Object &o){} const internal::Object& operator=(const Object &o) {return *this;} + int count_; double check_value_; }; Index: kernel/include/IMP/internal/ObjectPointer.h =================================================================== --- kernel/include/IMP/internal/ObjectPointer.h (revision 584) +++ kernel/include/IMP/internal/ObjectPointer.h (working copy) @@ -11,6 +11,7 @@ #include "../log.h" #include "Object.h" +#include "RefCountedObject.h" #include "../macros.h" #include "../exception.h" @@ -35,6 +36,16 @@ typedef ObjectPointer<O, RC> This; O* o_; + void set_pointer(O* p) { + if (RC) { + if (o_) disown(o_); + if (p) own(p); + o_=p; + } else { + o_=p; + } + } + // Enforce that ref counted objects are ref counted BOOST_STATIC_ASSERT((RC || !boost::is_base_of<RefCountedObject, O>::value)); @@ -48,34 +59,21 @@ } typedef bool (This::*unspecified_bool)() const; - void ref() { - if (RC && o_) o_->ref(); - } - - void unref() { - if (RC && o_) { - o_->unref(); - if (!o_->get_has_ref()) delete o_; - } - } - public: - ObjectPointer(const ObjectPointer &o): o_(o.o_) { - ref(); + ObjectPointer(const ObjectPointer &o): o_(NULL) { + set_pointer(o.o_); } ObjectPointer& operator=(const ObjectPointer &o){ - unref(); - o_=o.o_; - ref(); + set_pointer(o.o_); return *this; } ObjectPointer(): o_(NULL) {} - explicit ObjectPointer(O* o): o_(o) { - IMP_assert(o != NULL, "Can't initialize with NULL pointer"); - ref(); + explicit ObjectPointer(O* o): o_(NULL) { + IMP_assert(o, "Can't initialize with NULL pointer"); + set_pointer(o); } ~ObjectPointer(){ - unref(); + set_pointer(NULL); } const O& operator*() const { audit(); @@ -98,9 +96,7 @@ return o_; } void operator=(O* o) { - unref(); - o_=o; - ref(); + set_pointer(o); } IMP_COMPARISONS_1(o_); Index: kernel/include/IMP/internal/RefCountedObject.h =================================================================== --- kernel/include/IMP/internal/RefCountedObject.h (revision 584) +++ kernel/include/IMP/internal/RefCountedObject.h (working copy) @@ -22,12 +22,8 @@ //! Common base class for ref counted objects. /** Currently the only ref counted objects are particles. + This class acts as a tag rather than providing any functionality. - \note Due to weirdness in SWIG, the external objects - are responsible for deleting the ref counted object when - the ref count goes to zero. This will change once we have - a real solution for Python. - \internal */ class IMPDLLEXPORT RefCountedObject: public Object @@ -43,21 +39,12 @@ public: virtual ~RefCountedObject() { - IMP_assert(count_==0, "Deleting object which still has references"); + IMP_assert(!get_has_ref(), "Deleting object which still has references"); --live_objects_; } - void ref() { - assert_is_valid(); - ++P::count_; - } - void unref() { - assert_is_valid(); - IMP_assert(count_ != 0, "Too many unrefs on object"); - --P::count_; - } - static unsigned int get_number_of_live_objects() { + // for debugging purposes only return live_objects_; } }; @@ -68,9 +55,9 @@ struct Ref { template <class O> - static void eval(O) { - BOOST_STATIC_ASSERT((!boost::is_pointer<O>::value - || !boost::is_base_of<RefCountedObject, O >::value)); + static void eval(O* o) { + BOOST_STATIC_ASSERT((!boost::is_base_of<RefCountedObject, O >::value)); + IMP_LOG(VERBOSE, "Not refing particle " << o << std::endl); } }; @@ -79,7 +66,10 @@ { template <class O> static void eval(O* o) { - if (o) o->ref(); + IMP_LOG(VERBOSE, "Refing particle " << o->get_index() + << o->get_ref_count() << std::endl); + o->assert_is_valid(); + o->ref(); } }; @@ -87,9 +77,9 @@ struct UnRef { template <class O> - static void eval(O) { - BOOST_STATIC_ASSERT((!boost::is_pointer<O>::value - || !boost::is_base_of<RefCountedObject, O >::value)); + static void eval(O* o) { + BOOST_STATIC_ASSERT((!boost::is_base_of<RefCountedObject, O >::value)); + IMP_LOG(VERBOSE, "Not Unrefing object " << o << std::endl); } }; @@ -98,22 +88,24 @@ { template <class O> static void eval(O *o) { - if (o) { - o->unref(); - if (!o->get_has_ref()) { - delete o; - } + IMP_LOG(VERBOSE, "Unrefing particle " << o->get_index() + << " " << o->get_ref_count() << std::endl); + o->assert_is_valid(); + o->unref(); + if (!o->get_has_ref()) { + delete o; } } -}; + }; //! Can be called on any object and will only unref it if appropriate template <class O> void unref(O o) { - UnRef<(boost::is_pointer<O>::value - && boost::is_base_of<RefCountedObject, O >::value)>::eval(o); + BOOST_STATIC_ASSERT(!boost::is_pointer<O>::value); + /*IMP_LOG(VERBOSE, "NonUnRef called with nonpointer for " + << o << std::endl);*/ } @@ -121,11 +113,69 @@ template <class O> void ref(O o) { - Ref<(boost::is_pointer<O>::value - && boost::is_base_of<RefCountedObject, O >::value)>::eval(o); + BOOST_STATIC_ASSERT(!boost::is_pointer<O>::value); + /*IMP_LOG(VERBOSE, "NonRef count called with nonpointer for " + << o << std::endl);*/ } +//! Can be called on any object and will only unref it if appropriate +template <class O> +void unref(O* o) +{ + /*IMP_LOG(VERBOSE, "Unref count called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o << std::endl);*/ + UnRef<(boost::is_base_of<RefCountedObject, O >::value)>::eval(o); +} + +//! Can be called on any object and will only ref it if appropriate +template <class O> +void ref(O* o) +{ + /*IMP_LOG(VERBOSE, "Ref called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o << std::endl);*/ + Ref<(boost::is_base_of<RefCountedObject, O >::value)>::eval(o); +} + + +//! Can be called on any object and will only unref it if appropriate +template <class O> +void disown(O* o) +{ + /*IMP_LOG(VERBOSE, "Disown called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o << " " << o->get_ref_count() << std::endl);*/ + o->unref(); + if (!o->get_has_ref()) { + delete o; + } +} + + +//! Can be called on any object and will only ref it if appropriate +template <class O> +void own(O* o) +{ + /*IMP_LOG(VERBOSE, "Own called with " + << (boost::is_base_of<RefCountedObject, O >::value) + << " for " << o + << " " << o->get_ref_count() << std::endl);*/ + if (boost::is_base_of<RefCountedObject, O >::value) { + // no checks + } else { + IMP_check(!o->get_has_ref(), "Trying to own already owned but " + << "non-reference-counted object.", + ValueException("Already owned object")); + } + o->ref(); +} + + + + + } // namespace internal } // namespace IMP Index: kernel/test/states/test_nonbonded_list.py =================================================================== --- kernel/test/states/test_nonbonded_list.py (revision 584) +++ kernel/test/states/test_nonbonded_list.py (working copy) @@ -165,7 +165,7 @@ score= m.evaluate(False) self.assertEqual(score, 2*190, "Wrong score") - ps[3].set_is_active(False) + m.remove_particle(ps[3].get_index()) self.assert_(not ps[3].get_is_active(), "Particle not inactive") ps=None score= m.evaluate(False) Index: kernel/test/particles/test_refcount.py =================================================================== --- kernel/test/particles/test_refcount.py (revision 584) +++ kernel/test/particles/test_refcount.py (working copy) @@ -7,15 +7,23 @@ class RefCountTests(IMP.test.TestCase): """Test refcounting of particles""" + def setUp(self): + IMP.test.TestCase.setUp(self) + IMP.set_log_level(IMP.VERBOSE) + self.basenum= IMP.RefCountedObject.get_number_of_live_objects() + print "The base number of objects is " + str(self.basenum) + def _check_number(self, expected): print "Expected "+str(expected)\ - + " got " + str(IMP.RefCountedObject.get_number_of_live_objects()) - self.assertEqual(IMP.RefCountedObject.get_number_of_live_objects(), + + " got " + str(IMP.RefCountedObject.get_number_of_live_objects() + - self.basenum) + self.assertEqual(IMP.RefCountedObject.get_number_of_live_objects() - self.basenum, expected, "wrong number of particles") - def _test_simple(self): + def __test_simple(self): """Check that ref counting of particles works within python""" + # swig is broken so this needs to be skipped self._check_number(0) p= IMP.Particle() @@ -33,7 +41,7 @@ m=1 self._check_number(0) - def _test_removal(self): + def test_removal(self): """Check that ref counting works with removing particles""" self._check_number(0) m= IMP.Model() @@ -47,7 +55,7 @@ self._check_number(0) # This test does not work since swig refcounting is broken - def _test_round_trip(self): + def __test_round_trip(self): """test that tracking survives the round trip""" print "test that the round trip object doesn't delete it" @@ -101,24 +109,30 @@ self._check_number(0) - def _test_shared(self): - """Check that ref counting works shared particles""" + def test_shared(self): + """Check that ref counting works with shared particles""" print "max change" self._check_number(0) m= IMP.Model() p= IMP.Particle() + print "Add particle" pi= m.add_particle(p) + d= IMP.XYZDecorator.create(p) + d=0 mc= IMP.MaxChangeScoreState(IMP.XYZDecorator.get_xyz_keys()) + print "Add particle to mc" mc.add_particle(p) self._check_number(1) + print "Remove from model" m.remove_particle(pi) self._check_number(1) p=1 self._check_number(1) + print "Remove from mc" mc.clear_particles() self._check_number(0) - def _test_skip(self): + def test_skip(self): """Check that removed particles are skipped""" print "skipped" m= IMP.Model()

1 0

Particle removal and deactivation
by Daniel Russel 28 May '08

28 May '08

I can't find the imp-commit email for some reason. Anyway, there was a recent patch (581) changing a model::remove_particle to deactivating the particle. This change is wrong as the particles are currently reference counted in C++ and removing the particle from the model should do the right thing assuming there are no python references hanging around. If it does not, it is a bug in kernel code which should be fixed. In fact, the "Particle::set_is_active" method should probably go away.

3 7

Patch 583
by Daniel Russel 28 May '08

28 May '08

Patch 583 doesn't really make sense. Either it should become "if (table.contains()) table.remove" and remove always should be removed or remove_always should be fixed to do bounds checking. I think prefer the first.

2 1

Removing particle attributes
by Daniel Russel 25 May '08

25 May '08

The attached patch adds the ability to remove particle attributes. It also adds more thorough checking of NaNs in the float attributes. In addition, the rearrangement creates a natural iterator through the optimized float keys which the Optimizer now uses. Internally, it changes how attributes are store, replacing the valid bit with a sentinel value. This works nicely for floats and derivs where we can can use NaN and for particles where we can use NULL. It also reduces the memory usage for those by a bit since the bools couldn't be packed well before. For ints, it is a little less nice as it currently uses numeric_limits<int>::max as a sentinel. For strings it is a particular weird string. For both of those we can easily improve things if it becomes a problem (either due to people wanting to use int max as an attribute value or the memory allocation associated with the strings becoming significant). Index: kernel/include/IMP/internal/AttributeTable.h =================================================================== --- kernel/include/IMP/internal/AttributeTable.h (revision 576) +++ kernel/include/IMP/internal/AttributeTable.h (working copy) @@ -11,12 +11,14 @@ #include "../base_types.h" #include "../utility.h" #include "../log.h" +#include "ObjectPointer.h" #include <boost/iterator/filter_iterator.hpp> #include <boost/iterator/counting_iterator.hpp> #include <boost/scoped_array.hpp> #include <vector> +#include <limits> namespace IMP { @@ -25,25 +27,101 @@ namespace internal { +struct FloatAttributeTableTraits { + typedef Float Value; + typedef KeyBase<Float> Key; + static Float get_invalid() { + if (std::numeric_limits<Float>::has_quiet_NaN) { + return std::numeric_limits<Float>::quiet_NaN(); + } else if (std::numeric_limits<Float>::has_infinity) { + return std::numeric_limits<Float>::infinity(); + } else { + return std::numeric_limits<Float>::max(); + } + } + static bool get_is_valid(Float f) { + if (std::numeric_limits<Float>::has_quiet_NaN) { + return f==f; + } else { + return f!= get_invalid(); + } + } +}; + +struct IntAttributeTableTraits { + typedef Int Value; + typedef KeyBase<Int> Key; + static Int get_invalid() { + return std::numeric_limits<Int>::max(); + } + static bool get_is_valid(Int f) { + return f!= get_invalid(); + } +}; + +struct BoolAttributeTableTraits { + typedef bool Value; + typedef KeyBase<Float> Key; + static bool get_invalid() { + return false; + } + static bool get_is_valid(Int f) { + return f; + } +}; + +struct StringAttributeTableTraits { + typedef String Value; + typedef KeyBase<String> Key; + static Value get_invalid() { + return "This is an invalid string in IMP"; + } + static bool get_is_valid(String f) { + return f!= get_invalid(); + } +}; + +struct ParticleAttributeTableTraits { + typedef internal::ObjectPointer<Particle,true> Value; + typedef KeyBase<Particle*> Key; + static Value get_invalid() { + return Value(); + } + static bool get_is_valid(Value f) { + return f!= Value(); + } +}; + /** \internal If memory becomes a problem then either use a char table to look up actual entries or use perfect hashing. http://burtleburtle.net/bob/hash/perfect.html + + Actuall Cuckoo hashing is probably a better bet as that gets + high occupancy without large tables for the hash function. + + \note This version of the table uses certain values of the data + to singal that the entry is invalid. Setting an entry to these + values is a checked error. The values are specified by the + Traits::invalid entry. */ -template <class T, class VT> +template <class Traits> class AttributeTable { - typedef AttributeTable<T, VT> This; - struct Bin - { - bool first; - VT second; - Bin(): first(false){} - }; - typedef boost::scoped_array<Bin> Map; + typedef AttributeTable<Traits> This; + + typedef boost::scoped_array<typename Traits::Value> Map; Map map_; unsigned int size_; + typename Traits::Value* new_array(unsigned int asz) const { + typename Traits::Value* ret= new typename Traits::Value[asz]; + for (unsigned int i=0; i< asz; ++i) { + ret[i]= Traits::get_invalid(); + } + return ret; + } + void copy_from(unsigned int len, const Map &o) { IMP_assert(map_, "Internal error in attribute table"); IMP_assert(size_ >= len, "Table too small"); @@ -60,15 +138,25 @@ map_.reset(); } else { size_=std::max(nlen, 6U); - map_.reset(new Bin[size_]); + map_.reset(new_array(size_)); copy_from(olen, o); } //std::cout << " to " << size_ << " " << map_ << std::endl; } + void check_contains(typename Traits::Key k) const { + IMP_assert(size_ > k.get_index(), + "Attribute \"" << k.get_string() + << "\" not found in table."); + IMP_check(Traits::get_is_valid(map_[k.get_index()]), + "Attribute \"" << k.get_string() + << "\" not found in table.", + IndexException("Invalid attribute")); + } + public: - typedef VT Value; - typedef KeyBase<T> Key; + typedef typename Traits::Value Value; + typedef typename Traits::Key Key; AttributeTable(): size_(0){} AttributeTable(const This &o): size_(0) { //std::cout << "Copy constructor called from " << o.map_ << std::endl; @@ -87,28 +175,43 @@ return *this; } const Value get_value(Key k) const { - IMP_assert(contains(k), - "Attribute \"" << k.get_string() - << "\" not found in table."); - return map_[k.get_index()].second; + check_contains(k); + return map_[k.get_index()]; } - Value& get_value(Key k) { - IMP_assert(contains(k), - "Attribute \"" << k.get_string() - << "\" not found in table."); - return map_[k.get_index()].second; + void set_value(Key k, Value v) { + check_contains(k); + IMP_check(Traits::get_is_valid(v), + "Cannot set value of attribute to " << v, + ValueException("Invalid value for attribute")); + map_[k.get_index()] = v; } - void insert(Key k, Value v); + void insert(Key k, Value v) { + IMP_assert(!contains(k), + "Trying to add attribute \"" << k.get_string() + << "\" twice"); + insert_always(k, v); + } + void insert_always(Key k, Value v); + void remove(Key k) { + check_contains(k); + map_[k.get_index()]= Traits::get_invalid(); + } + + void remove_always(Key k) { + map_[k.get_index()]= Traits::get_invalid(); + } + + bool contains(Key k) const { IMP_assert(k != Key(), "Can't search for default key"); return k.get_index() < size_ - && map_[k.get_index()].first; + && Traits::get_is_valid(map_[k.get_index()]); } @@ -143,57 +246,52 @@ KeyIterator(Key(size_))); } - - unsigned int get_heap_memory_usage() const { - return size_*sizeof(Bin); - } - }; -IMP_OUTPUT_OPERATOR_2(AttributeTable) +IMP_OUTPUT_OPERATOR_1(AttributeTable) -template <class T, class VT> -inline void AttributeTable<T, VT>::insert(Key k, Value v) +template <class Traits> +inline void AttributeTable<Traits>::insert_always(Key k, Value v) { /*std::cout << "Insert " << k << " in v of size " << size_ << " " << map_ << " " << k.get_index() << std::endl;*/ IMP_assert(k != Key(), "Can't insert default key"); + IMP_check(Traits::get_is_valid(v), + "Trying to insert invalid value for attribute " + << v << " into attribute " << k, + ValueException("Invalid attribute value")); if (size_ <= k.get_index()) { - boost::scoped_array<Bin> old; + boost::scoped_array<Value> old; swap(old, map_); realloc(size_, old, k.get_index()+1); } - IMP_assert(!map_[k.get_index()].first, - "Trying to add attribute \"" << k.get_string() - << "\" twice"); - map_[k.get_index()].second= v; - map_[k.get_index()].first= true; - IMP_assert(contains(k), "Something is broken"); + map_[k.get_index()]= v; } - template <class T, class VT> - inline void AttributeTable<T, VT>::show(std::ostream &out, - const char *prefix) const + +template <class Traits> +inline void AttributeTable<Traits>::show(std::ostream &out, + const char *prefix) const { for (unsigned int i=0; i< size_; ++i) { - if (map_[i].first) { + if (Traits::get_is_valid(map_[i])) { out << prefix; out << Key(i).get_string() << ": "; - out << map_[i].second; + out << map_[i]; out << std::endl; } } } -template <class T, class VT> -inline std::vector<typename AttributeTable<T, VT>::Key> - AttributeTable<T, VT>::get_keys() const +template <class Traits> +inline std::vector<typename Traits::Key> + AttributeTable<Traits>::get_keys() const { std::vector<Key> ret(attribute_keys_begin(), attribute_keys_end()); return ret; Index: kernel/include/IMP/Particle.h =================================================================== --- kernel/include/IMP/Particle.h (revision 576) +++ kernel/include/IMP/Particle.h (working copy) @@ -26,25 +26,6 @@ namespace internal { -struct IMPDLLEXPORT FloatData -{ - bool is_optimized; - Float value; - Float derivative; - FloatData(Float v, bool opt): is_optimized(opt), value(v), derivative(0){} - FloatData():is_optimized(false), value(std::numeric_limits<Float>::max()), - derivative(std::numeric_limits<Float>::max()){} -}; - -inline std::ostream &operator<<(std::ostream &out, const FloatData &d) -{ - out << d.value; - if (d.is_optimized) { - out << " (" << d.derivative << ")"; - } - return out; -} - template <class It> void check_particles_active(It b, It e, std::string msg) { @@ -61,17 +42,29 @@ //! Class to handle individual model particles. /** This class contains particle methods and indexes to particle attributes. - Particles cannot be deleted once they are added to a model, but they can - be deactivated (with their set_is_active method) after which they play no - role in the scoring (it is illegal to try to evaluate a restraint on an - inactive particle). To merely prevent a particle from moving during + To merely prevent a particle from moving during optimization, mark all of its attributes as being non-optimizable (set_is_optimized method). + \ingroup kernel */ class IMPDLLEXPORT Particle : public internal::RefCountedObject { friend class Model; + + typedef internal::AttributeTable<internal::FloatAttributeTableTraits> + FloatTable; + typedef internal::AttributeTable<internal::FloatAttributeTableTraits> + DerivativeTable; + typedef internal::AttributeTable<internal::BoolAttributeTableTraits> + OptimizedTable; + typedef internal::AttributeTable<internal::IntAttributeTableTraits> + IntTable; + typedef internal::AttributeTable<internal::StringAttributeTableTraits> + StringTable; + typedef internal::AttributeTable<internal::ParticleAttributeTableTraits> + ParticleTable; + public: @@ -100,6 +93,12 @@ void add_attribute(FloatKey name, const Float value, const bool is_optimized = false); + //! Remove a Float attribute from this particle. + /** \param[in] name Name of the attribute being added. + */ + void remove_attribute(FloatKey name); + + //! Does particle have a Float attribute with the given name. /** \param[in] name Name of the attribute being checked. \return true if Float attribute exists in this particle. @@ -147,6 +146,11 @@ */ void add_attribute(IntKey name, const Int value); + //! Remove a Int attribute from this particle. + /** \param[in] name Name of the attribute being added. + */ + void remove_attribute(IntKey name); + //! Does particle have an Int attribute with the given name. /** \param[in] name Name of the attribute being checked. \return true if Int attribute exists in this particle. @@ -173,6 +177,12 @@ */ void add_attribute(StringKey name, const String value); + //! Remove a String attribute from this particle. + /** \param[in] name Name of the attribute being added. + */ + void remove_attribute(StringKey name); + + //! Does particle have a String attribute with the given name. /** \param[in] name Name of the attribute being checked. \return true if Int attribute exists in this particle. @@ -200,6 +210,11 @@ */ void add_attribute(ParticleKey name, Particle* value); + //! Remove a Particle attribute from this particle. + /** \param[in] name Name of the attribute being added. + */ + void remove_attribute(ParticleKey name); + //! Does particle have a Particle attribute with the given name. /** \param[in] name Name of the attribute being checked. \return true if Particle attribute exists in this particle. @@ -256,72 +271,78 @@ which seems inelegant. */ std::vector<FloatKey> get_float_attributes() const { - return float_indexes_.get_keys(); + return floats_.get_keys(); } //! See get_float_attributes std::vector<IntKey> get_int_attributes() const { - return int_indexes_.get_keys(); + return ints_.get_keys(); } //! See get_float_attributes std::vector<StringKey> get_string_attributes() const { - return string_indexes_.get_keys(); + return strings_.get_keys(); } //! See get_particle_attributes std::vector<ParticleKey> get_particle_attributes() const { - return particle_indexes_.get_keys(); + return particles_.get_keys(); } //! An iterator through the keys of the float attributes of this particle - typedef internal::AttributeTable<Float, - internal::FloatData>::AttributeKeyIterator + typedef FloatTable::AttributeKeyIterator FloatKeyIterator; //! Iterate through the keys of float attributes of the particle FloatKeyIterator float_keys_begin() const { - return float_indexes_.attribute_keys_begin(); + return floats_.attribute_keys_begin(); } FloatKeyIterator float_keys_end() const { - return float_indexes_.attribute_keys_end(); + return floats_.attribute_keys_end(); } + //! An iterator through the keys of the derivative attributes of this particle + typedef OptimizedTable::AttributeKeyIterator + OptimizedKeyIterator; + //! Iterate through the keys of float attributes of the particle + OptimizedKeyIterator optimized_keys_begin() const { + return optimizeds_.attribute_keys_begin(); + } + OptimizedKeyIterator optimized_keys_end() const { + return optimizeds_.attribute_keys_end(); + } + + //! An iterator through the keys of the int attributes of this particle - typedef internal::AttributeTable<Int, Int> - ::AttributeKeyIterator IntKeyIterator; + typedef IntTable::AttributeKeyIterator IntKeyIterator; //! Iterate through the keys of int attributes of the particle IntKeyIterator int_keys_begin() const { - return int_indexes_.attribute_keys_begin(); + return ints_.attribute_keys_begin(); } IntKeyIterator int_keys_end() const { - return int_indexes_.attribute_keys_end(); + return ints_.attribute_keys_end(); } //! An iterator through the keys of the string attributes of this particle - typedef internal::AttributeTable<String, String> - ::AttributeKeyIterator StringKeyIterator; + typedef StringTable::AttributeKeyIterator StringKeyIterator; //! Iterate through the keys of string attributes of the particle StringKeyIterator string_keys_begin() const { - return string_indexes_.attribute_keys_begin(); + return strings_.attribute_keys_begin(); } StringKeyIterator string_keys_end() const { - return string_indexes_.attribute_keys_end(); + return strings_.attribute_keys_end(); } //! An iterator through the keys of the Particle attributes of this particle - typedef internal::AttributeTable<Particle*, - internal::ObjectPointer<Particle, true> > - ::AttributeKeyIterator ParticleKeyIterator; + typedef ParticleTable::AttributeKeyIterator ParticleKeyIterator; //! Iterate through the keys of Particle attributes of the particle ParticleKeyIterator particle_keys_begin() const { - return particle_indexes_.attribute_keys_begin(); + return particles_.attribute_keys_begin(); } ParticleKeyIterator particle_keys_end() const { - return particle_indexes_.attribute_keys_end(); + return particles_.attribute_keys_end(); } - protected: void zero_derivatives(); @@ -335,14 +356,18 @@ bool is_active_; // float attributes associated with the particle - internal::AttributeTable<Float, internal::FloatData> float_indexes_; + FloatTable floats_; + // float attributes associated with the particle + FloatTable derivatives_; + // Whether a given float is optimized or not + OptimizedTable optimizeds_; + // int attributes associated with the particle - internal::AttributeTable<Int, Int> int_indexes_; + IntTable ints_; // string attributes associated with the particle - internal::AttributeTable<String, String> string_indexes_; + StringTable strings_; // particle attributes associated with the particle - internal::AttributeTable<Particle*, internal::ObjectPointer<Particle,true> > - particle_indexes_; + ParticleTable particles_; ParticleIndex pi_; }; @@ -354,7 +379,7 @@ inline bool Particle::has_attribute(FloatKey name) const { - return float_indexes_.contains(name); + return floats_.contains(name); } @@ -363,36 +388,45 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return float_indexes_.get_value(name).value; + return floats_.get_value(name); } inline Float Particle::get_derivative(FloatKey name) const { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return float_indexes_.get_value(name).derivative; + return derivatives_.get_value(name); } inline void Particle::set_value(FloatKey name, Float value) { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - IMP_assert(value==value, "Can't set value to NaN"); - float_indexes_.get_value(name).value= value; + floats_.set_value(name, value); } inline bool Particle::get_is_optimized(FloatKey name) const { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return float_indexes_.get_value(name).is_optimized; + IMP_check(floats_.contains(name), "get_is_optimized called " + << "with invalid attribute" << name, + IndexException("Invalid float attribute")); + return optimizeds_.contains(name); } inline void Particle::set_is_optimized(FloatKey name, bool tf) { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - float_indexes_.get_value(name).is_optimized=tf; + IMP_check(floats_.contains(name), "set_is_optimized called " + << "with invalid attribute" << name, + IndexException("Invalid float attribute")); + if (tf) { + optimizeds_.insert_always(name, true); + } else { + optimizeds_.remove_always(name); + } } inline void Particle::add_to_derivative(FloatKey name, Float value, @@ -401,14 +435,14 @@ IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); IMP_assert(value==value, "Can't add NaN to derivative in particle " << *this); - float_indexes_.get_value(name).derivative+= da(value); + derivatives_.set_value(name, derivatives_.get_value(name)+ da(value)); } inline bool Particle::has_attribute(IntKey name) const { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return int_indexes_.contains(name); + return ints_.contains(name); } @@ -417,7 +451,7 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return int_indexes_.get_value(name); + return ints_.get_value(name); } @@ -425,14 +459,14 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - int_indexes_.get_value(name)= value; + ints_.set_value(name, value); } inline bool Particle::has_attribute(StringKey name) const { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return string_indexes_.contains(name); + return strings_.contains(name); } @@ -441,14 +475,14 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return string_indexes_.get_value(name); + return strings_.get_value(name); } inline void Particle::set_value(StringKey name, String value) { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - string_indexes_.get_value(name)= value; + strings_.set_value(name, value); } @@ -456,7 +490,7 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return particle_indexes_.contains(name); + return particles_.contains(name); } @@ -465,7 +499,7 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - return particle_indexes_.get_value(name).get(); + return particles_.get_value(name).get(); } @@ -473,7 +507,7 @@ { IMP_check(get_is_active(), "Do not touch inactive particles", InactiveParticleException()); - particle_indexes_.get_value(name)= value; + particles_.set_value(name, internal::ObjectPointer<Particle, true>(value)); } @@ -482,34 +516,60 @@ { IMP_assert(model_ , "Particle must be added to Model before attributes are added"); - float_indexes_.insert(name, internal::FloatData(value, is_optimized)); + floats_.insert(name, value); + derivatives_.insert(name, 0); + if (is_optimized) { + optimizeds_.insert(name, true); + } } +void inline Particle::remove_attribute(FloatKey name) +{ + floats_.remove(name); + derivatives_.remove(name); + optimizeds_.remove_always(name); +} + void inline Particle::add_attribute(IntKey name, const Int value) { IMP_assert(model_, "Particle must be added to Model before attributes are added"); - int_indexes_.insert(name, value); + ints_.insert(name, value); } +void inline Particle::remove_attribute(IntKey name) +{ + ints_.remove(name); +} + void inline Particle::add_attribute(StringKey name, const String value) { IMP_assert(model_, "Particle must be added to Model before attributes are added"); - string_indexes_.insert(name, value); + strings_.insert(name, value); } +void inline Particle::remove_attribute(StringKey name) +{ + strings_.remove(name); +} + void inline Particle::add_attribute(ParticleKey name, Particle* value) { IMP_assert(model_, "Particle must be added to Model before attributes are added"); - particle_indexes_.insert(name, + particles_.insert(name, internal::ObjectPointer<Particle, true>(value)); } +void inline Particle::remove_attribute(ParticleKey name) +{ + particles_.remove(name); +} + } // namespace IMP #endif /* __IMP_PARTICLE_H */ Index: kernel/include/IMP/Optimizer.h =================================================================== --- kernel/include/IMP/Optimizer.h (revision 576) +++ kernel/include/IMP/Optimizer.h (working copy) @@ -70,7 +70,6 @@ void update_states(); //! An index to an optimized particle attribute - class FloatIndexInderator; struct FloatIndex { /** @@ -79,7 +78,7 @@ friend class Optimizer; friend class FloatIndexIterator; Model::ParticleIterator p_; - Particle::FloatKeyIterator fk_; + Particle::OptimizedKeyIterator fk_; FloatIndex(Model::ParticleIterator p): p_(p){} public: FloatIndex() {} @@ -96,20 +95,19 @@ mutable FloatIndex i_; void search_valid() const { - while (i_.fk_ == (*i_.p_)->float_keys_end() - || !(*i_.p_)->get_is_optimized(*i_.fk_)) { - if (i_.fk_ == (*i_.p_)->float_keys_end()) { + while (i_.fk_ == (*i_.p_)->optimized_keys_end()) { + if (i_.fk_ == (*i_.p_)->optimized_keys_end()) { ++i_.p_; if (i_.p_== pe_) return; else { - i_.fk_= (*i_.p_)->float_keys_begin(); + i_.fk_= (*i_.p_)->optimized_keys_begin(); } } else { ++i_.fk_; } } IMP_assert(i_.p_ != pe_, "Should have just returned"); - IMP_assert(i_.fk_ != (*i_.p_)->float_keys_end(), + IMP_assert(i_.fk_ != (*i_.p_)->optimized_keys_end(), "Broken iterator end"); IMP_assert((*i_.p_)->get_is_optimized(*i_.fk_), "Why did the loop end?"); @@ -122,7 +120,7 @@ FloatIndexIterator(Model::ParticleIterator pc, Model::ParticleIterator pe): pe_(pe), i_(pc) { if (pc != pe) { - i_.fk_= (*pc)->float_keys_begin(); + i_.fk_= (*pc)->optimized_keys_begin(); search_valid(); } } @@ -162,7 +160,7 @@ FloatIndexIterator float_indexes_end() { return FloatIndexIterator(model_->particles_end(), - model_->particles_end()); + model_->particles_end()); } //! Set the value of an optimized attribute Index: kernel/test/particles/test_particles.py =================================================================== --- kernel/test/particles/test_particles.py (revision 576) +++ kernel/test/particles/test_particles.py (working copy) @@ -68,6 +68,28 @@ p.set_is_active(False) self.assertEqual(p.get_is_active(), False) + def _test_add_remove(self, p, ak, v): + p.add_attribute(ak, v) + self.assert_(p.has_attribute(ak)) + p.remove_attribute(ak) + self.assert_(not p.has_attribute(ak)) + + def test_remove_attributes(self): + """Test that attributes can be removed""" + p=self.particles[0] + fk= IMP.FloatKey("to_remove") + p.add_attribute(fk, 0, False) + self.assert_(p.has_attribute(fk)) + self.assert_(not p.get_is_optimized(fk)) + p.set_is_optimized(fk, True) + self.assert_(p.get_is_optimized(fk)) + p.set_is_optimized(fk, False) + self.assert_(not p.get_is_optimized(fk)) + self._test_add_remove(p, IMP.FloatKey("something"), 1.0) + self._test_add_remove(p, IMP.StringKey("something"), "Hello") + self._test_add_remove(p, IMP.IntKey("something"), 1) + self._test_add_remove(p, IMP.ParticleKey("something"), p) + def test_derivatives(self): """Test get/set of derivatives""" p = self.particles[0] Index: kernel/src/Model.cpp =================================================================== --- kernel/src/Model.cpp (revision 576) +++ kernel/src/Model.cpp (working copy) @@ -75,6 +75,7 @@ "Begin evaluate restraints " << (calc_derivs?"with derivatives":"without derivatives") << std::endl); + for (RestraintIterator it = restraints_begin(); it != restraints_end(); ++it) { IMP_CHECK_OBJECT(*it); @@ -87,34 +88,23 @@ IMP_LOG(TERSE, "Restraint score is " << tscore << std::endl); score+= tscore; } + IMP_LOG(TERSE, "End evaluate restraints." << std::endl); IMP_LOG(TERSE, "Begin after_evaluate of ScoreStates " << std::endl); + for (ScoreStateIterator it = score_states_begin(); it != score_states_end(); ++it) { IMP_CHECK_OBJECT(*it); (*it)->after_evaluate(iteration_, accpt); IMP_LOG(VERBOSE, "." << std::flush); } + IMP_LOG(TERSE, "End after_evaluate of ScoreStates." << std::endl); 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 576) +++ kernel/src/Particle.cpp (working copy) @@ -44,7 +44,7 @@ void Particle::zero_derivatives() { for (FloatKeyIterator it= float_keys_begin(); it != float_keys_end(); ++it) { - float_indexes_.get_value(*it).derivative=0; + derivatives_.set_value(*it, 0); } } @@ -63,13 +63,17 @@ if (get_model() != NULL) { out << inset << inset << "float attributes:" << std::endl; - float_indexes_.show(out, " "); + floats_.show(out, " "); out << inset << inset << "int attributes:" << std::endl; - int_indexes_.show(out, " "); + ints_.show(out, " "); out << inset << inset << "string attributes:" << std::endl; - string_indexes_.show(out, " "); + strings_.show(out, " "); + + out << inset << inset << "particle attributes:" << std::endl; + particles_.show(out, " "); + } } Index: kernel/src/internal/graph_base.cpp =================================================================== --- kernel/src/internal/graph_base.cpp (revision 576) +++ kernel/src/internal/graph_base.cpp (working copy) @@ -74,7 +74,7 @@ p[i]->set_value(graph_get_edge_key(j+shift, d), v); } } - p[i]->set_value(graph_get_edge_key(nc-1, d), NULL); + p[i]->remove_attribute(graph_get_edge_key(nc-1, d)); IMP_assert(shift==-1, "no edge found"); IMP_assert(nc > 0, "Too few edges"); p[i]->set_value(d.num_edges_key_, nc-1);

2 3

Changing white space in internal/Vector.h
by Daniel Russel 23 May '08

23 May '08

Added some white space to make it easier to read. The email to commit system is definitely overkill for this :-) Index: kernel/include/IMP/internal/Vector.h =================================================================== --- kernel/include/IMP/internal/Vector.h (revision 573) +++ kernel/include/IMP/internal/Vector.h (working copy) @@ -33,30 +33,37 @@ public: typedef std::vector<D> P; Vector(){} + template <class It> Vector(It b, It e) { insert(P::end(), b, e); } + Vector(const P &p) { insert(P::end(), p.begin(), p.end()); } + ~Vector(){clear();} + const D& operator[](unsigned int i) const { IMP_check(i < P::size(), "Index " << i << " out of range", IndexException("")); return P::operator[](i); } + D& operator[](unsigned int i) { IMP_check(i < P::size(), "Index " << i << " out of range", IndexException("")); return P::operator[](i); } + void erase(typename P::iterator it) { unref(*it); P::erase(it); } + void erase(typename P::iterator b, typename P::iterator e) { for (typename P::iterator c= b; c != e; ++c) { @@ -64,21 +71,25 @@ } P::erase(b,e); } + unsigned int push_back(D d) { ref(d); P::push_back(d); return P::size()-1; } + void pop_back() { unref(P::back()); P::pop_back(); } + void clear() { for (typename P::iterator it= P::begin(); it != P::end(); ++it) { unref(*it); } P::clear(); } + template <class It> void insert(typename P::iterator it, It b, It e) { for (It c= b; c != e; ++c) {

1 0

Move exception code
by Daniel Russel 23 May '08

23 May '08

This patch moves the code called on check and exception failures to exception.cpp rather than Log.cpp. Index: kernel/include/IMP/exception.h =================================================================== --- kernel/include/IMP/exception.h (revision 573) +++ kernel/include/IMP/exception.h (working copy) @@ -134,13 +134,13 @@ { //! This is just here so you can catch errors more easily in the debugger -/** Break on Log.cpp:19 to catch assertion failures. +/** Break on exception.cpp:31 to catch assertion failures. \ingroup assert */ IMPDLLEXPORT void assert_fail(); //! Here so you can catch check failures more easily in the debugger -/** Break on Log.cpp:22 to catch check failures. +/** Break on exception.cpp:35 to catch check failures. \ingroup assert */ IMPDLLEXPORT void check_fail(); @@ -178,12 +178,13 @@ \param[in] exception Throw the object constructed by this expression. \ingroup assert */ -#define IMP_check(expr, message, exception) \ - do { \ +#define IMP_check(expr, message, exception) \ + do { \ if (IMP::get_check_level() >= IMP::CHEAP && !(expr)) { \ - IMP_ERROR(message); \ - throw exception; \ - } \ + IMP_ERROR(message); \ + IMP::internal::check_fail(); \ + throw exception; \ + } \ } while (false) //! A runtime failure for IMP. Index: kernel/src/exception.cpp =================================================================== --- kernel/src/exception.cpp (revision 573) +++ kernel/src/exception.cpp (working copy) @@ -27,4 +27,13 @@ return check_mode; } +namespace internal { + void assert_fail() { + throw ErrorException(); + } + + void check_fail() { + } +} + } // namespace IMP Index: kernel/src/Log.cpp =================================================================== --- kernel/src/Log.cpp (revision 573) +++ kernel/src/Log.cpp (working copy) @@ -14,13 +14,4 @@ /* Initialize singleton pointer to NULL */ Log* Log::logpt_ = NULL; - -namespace internal { - void assert_fail() { - throw ErrorException(); - } - - void check_fail() { - } -} } // namespace IMP

1 0

Test code for cover bonds
by Daniel Russel 22 May '08

22 May '08

Here is some test code for the cover bonds score state. I forgot to include it before. Index: kernel/test/states/test_cover_bonds.py =================================================================== --- kernel/test/states/test_cover_bonds.py (revision 0) +++ kernel/test/states/test_cover_bonds.py (revision 0) @@ -0,0 +1,71 @@ +import unittest +import IMP +import IMP.test + +rk = IMP.FloatKey("radius") + +class TestBondCover(IMP.test.TestCase): + """Tests for BondCover""" + + def _set_up_stuff(self, n): + # return [model, particles, bonds] + m= IMP.Model() + ps= IMP.Particles() + for i in range(0,n): + p= IMP.Particle() + m.add_particle(p) + d= IMP.XYZDecorator.create(p) + d.randomize_in_box(IMP.Vector3D(0,0,0), + IMP.Vector3D(10,10,10)) + ps.append(p) + bds= [] + for i in range(1,n): + ba= IMP.BondedDecorator.create(ps[i-1]) + bb= IMP.BondedDecorator.create(ps[i]) + bds.append(IMP.custom_bond(ba, bb, 10, 1)) + bl= IMP.BondDecoratorListScoreState(ps) + ss= IMP.CoverBondsScoreState(bl, rk) + m.add_score_state(bl) + m.add_score_state(ss) + return [m, ps, bds] + + + + + def test_set_position(self): + """Make sure that bond cover coordinates are correct""" + IMP.set_log_level(IMP.VERBOSE) + [m,ps, bds]= self._set_up_stuff(10) + m.evaluate(False) + for b in bds: + b.get_particle().show() + ba= b.get_bonded(0) + bb= b.get_bonded(1) + da= IMP.XYZDecorator.cast(ba.get_particle()) + db= IMP.XYZDecorator.cast(bb.get_particle()) + dc= IMP.XYZDecorator.cast(b.get_particle()) + center= [(da.get_x()+ db.get_x()) /2.0, + (da.get_y()+ db.get_y()) /2.0, + (da.get_z()+ db.get_z()) /2.0] + r= ((center[0]- da.get_x())**2 + +(center[1]- da.get_y())**2 + +(center[2]- da.get_z())**2)**.5 + print r + print center + da.show() + db.show() + dc.show() + self.assertInTolerance(r, b.get_particle().get_value(rk), r*.1, + "Radius is not close enough") + self.assertInTolerance(center[0], dc.get_x(), .1, + "X is not close enough") + self.assertInTolerance(center[1], dc.get_y(), .1, + "Y is not close enough") + self.assertInTolerance(center[2], dc.get_z(), .1, + "Z is not close enough") + + + + +if __name__ == '__main__': + unittest.main()

1 0

patches
by Daniel Russel 21 May '08

21 May '08

The first just fixes some minor errors in the LIST macros. The second cleans up on the MoverBase, making it use the LIST macros. In addition, the order of the arguments on the movers has been changed to make particles last as in the restraints. Index: kernel/include/IMP/macros.h =================================================================== --- kernel/include/IMP/macros.h (revision 569) +++ kernel/include/IMP/macros.h (working copy) @@ -240,7 +240,7 @@ /** \short Add several objects. \param[in] obj a vector of pointers */ \ - void add_##lcname##s(const std::vector<Ucname*>& obj); \ + void add_##lcname##s(const std::vector<Data>& obj); \ /** \short Clear the contents of the container */ \ void clear_##lcname##s(); \ /** \short return the number of objects*/ \ @@ -277,7 +277,7 @@ IndexType index= lcname##_vector_.push_back(obj); \ Init_obj; \ Onchanged; \ - if (false) std::cout << index; \ + if (false) {index=index; obj=obj;}; \ return index; \ } \ void Class::add_##lcname##s(const std::vector<Data> &objs) { \ @@ -288,7 +288,7 @@ Data obj= lcname##_vector_[osz+i]; \ IndexType index(osz+i); \ Init_obj; \ - if (false) std::cout << *obj << index; \ + if (false) {obj=obj; index=index;} \ } \ Onchanged; \ } \ Index: kernel/include/IMP/optimizers/MoverBase.h =================================================================== --- kernel/include/IMP/optimizers/MoverBase.h (revision 569) +++ kernel/include/IMP/optimizers/MoverBase.h (working copy) @@ -10,6 +10,7 @@ #include "Mover.h" #include "../Particle.h" +#include "../macros.h" #include <vector> @@ -28,9 +29,6 @@ { std::vector<Floats> floats_; std::vector<Ints> ints_; - FloatKeys fkeys_; - IntKeys ikeys_; - Particles ps_; public: virtual void accept_move(){} virtual void reject_move(); @@ -39,59 +37,24 @@ */ virtual void propose_move(float f); + IMP_LIST(public, Particle, particle, Particle*); + IMP_LIST(public, FloatKey, float_key, FloatKey); + IMP_LIST(public, IntKey, int_key, IntKey); + protected: //! implement this method to propose a move /** See NormalMover for a simple example. */ virtual void generate_move(float f)=0; - //! Add to the set of particles being controlled - void add_particles(const Particles &ps) { - ps_.insert(ps_.end(), ps.begin(), ps.end()); - } - //! Clear the set of particles being controlled - void clear_particles() { - ps_.clear(); - } - - //! Get the number of particles - unsigned int number_of_particles() const { - return ps_.size(); - } - Particle *get_particle(unsigned int i) const { - IMP_check(i< ps_.size(), "Bad index in MoverBase::get_particle", - IndexException("Particle index out of range in mover base")); - return ps_[i]; - } - - //! Add an attribute - unsigned int add_key(FloatKey k) { - fkeys_.push_back(k); - return fkeys_.size()-1; - } - //! Add an attribute - unsigned int add_key(IntKey k) { - ikeys_.push_back(k); - return ikeys_.size()-1; - } - - unsigned int number_of_float_keys() const {return fkeys_.size();} - unsigned int number_of_int_keys() const {return ikeys_.size();} - - - //! Get the value of a controlled attribute /** \param [in] i The index of the particle. \param [in] j The index of the attribute. */ Float get_float(unsigned int i, unsigned int j) const { - IMP_assert(i < ps_.size(), - "Out of range controlled attribute in guard"); - IMP_assert(j < fkeys_.size(), - "Out of range controlled attribute in guard"); - IMP_assert(ps_.size() == floats_.size(), + IMP_assert(number_of_particles() == floats_.size(), "Only call get_float from within generate_proposal"); - return get_particle(i)->get_value(fkeys_[j]); + return get_particle(i)->get_value(get_float_key(j)); } //! Get an int attribute value @@ -99,23 +62,19 @@ \param [in] j The index of the attribute. */ Int get_int(unsigned int i, unsigned int j) const { - IMP_assert(i < ps_.size(), - "Out of range controlled attribute in guard"); - IMP_assert(j < ikeys_.size(), - "Out of range controlled attribute in guard"); - IMP_assert(ps_.size() == ints_.size(), - "Only call get_float from within generate_proposal"); - - return get_particle(i)->get_value(ikeys_[j]); + IMP_assert(number_of_particles() == ints_.size(), + "Only call get_int from within generate_proposal"); + return get_particle(i)->get_value(get_int_key(j)); } + //! Propose a value /** \param[in] i The index of the particle. \param[in] j The index of the key \param[in] t The value to propose */ void propose_value(unsigned int i, unsigned int j, Float t) { - if (get_particle(i)->get_is_optimized(fkeys_[j])) { - get_particle(i)->set_value(fkeys_[j], t); + if (get_particle(i)->get_is_optimized(get_float_key(j))) { + get_particle(i)->set_value(get_float_key(j), t); } } //! Propose a value @@ -124,7 +83,7 @@ \param[in] t The value to propose */ void propose_value(unsigned int i, unsigned int j, Int t) { - get_particle(i)->set_value(ikeys_[j], t); + get_particle(i)->set_value(get_int_key(j), t); } MoverBase(){} Index: kernel/include/IMP/optimizers/movers/NormalMover.h =================================================================== --- kernel/include/IMP/optimizers/movers/NormalMover.h (revision 569) +++ kernel/include/IMP/optimizers/movers/NormalMover.h (working copy) @@ -21,16 +21,13 @@ class IMPDLLEXPORT NormalMover :public MoverBase { public: - /** \param[in] ps The particles to perturb. - \param[in] vars The variables to use (normally the keys for x,y,z) + /** \param[in] vars The variables to use (normally the keys for x,y,z) \param[in] sigma The standard deviation to use. + \param[in] ps The particles to perturb. */ - NormalMover(const Particles &ps, const FloatKeys &vars, - Float sigma); - void set_particles(const Particles &ps) { - MoverBase::clear_particles(); - MoverBase::add_particles(ps); - } + NormalMover(const FloatKeys &vars, + Float sigma, const Particles &ps=Particles()); + void set_sigma(Float sigma) { IMP_check(sigma > 0, "Sigma must be positive", ValueException("Negative sigma")); Index: kernel/include/IMP/optimizers/movers/BallMover.h =================================================================== --- kernel/include/IMP/optimizers/movers/BallMover.h (revision 569) +++ kernel/include/IMP/optimizers/movers/BallMover.h (working copy) @@ -26,12 +26,12 @@ public: /** The attributes are perturbed within a pall whose dimensionality is given by the number of attributes and radius by the given value. - \param[in] ps The particles to perturb. \param[in] vars The variables to use (normally the keys for x,y,z) \param[in] radius The radius deviation to use. + \param[in] ps The particles to perturb. */ - BallMover(const Particles &ps, const FloatKeys &vars, - Float radius); + BallMover(const FloatKeys &vars, + Float radius, const Particles &ps); //! void set_radius(Float radius) { IMP_check(radius > 0, "The radius must be positive", Index: kernel/src/optimizers/MoverBase.cpp =================================================================== --- kernel/src/optimizers/MoverBase.cpp (revision 569) +++ kernel/src/optimizers/MoverBase.cpp (working copy) @@ -10,21 +10,25 @@ namespace IMP { +IMP_LIST_IMPL(MoverBase, Particle, particle, Particle*,,); +IMP_LIST_IMPL(MoverBase, FloatKey, float_key, FloatKey,,); +IMP_LIST_IMPL(MoverBase, IntKey, int_key, IntKey,,); + void MoverBase::propose_move(float f) { - if (!fkeys_.empty()) { - floats_.resize(number_of_particles(), Floats(fkeys_.size(), 0)); + if (number_of_float_keys() != 0) { + floats_.resize(number_of_particles(), Floats(number_of_float_keys(), 0)); } - if (!ikeys_.empty()) { - ints_.resize(number_of_particles(), Ints(ikeys_.size(), 0)); + if (number_of_int_keys() != 0) { + ints_.resize(number_of_particles(), Ints(number_of_int_keys(), 0)); } for (unsigned int i=0; i< number_of_particles(); ++i) { Particle *p = get_particle(i); - for (unsigned int j=0; j< fkeys_.size(); ++j) { - floats_[i][j]= p->get_value(fkeys_[j]); + for (unsigned int j=0; j< number_of_float_keys(); ++j) { + floats_[i][j]= p->get_value(get_float_key(j)); } - for (unsigned int j=0; j< ikeys_.size(); ++j) { - ints_[i][j]= p->get_value(ikeys_[j]); + for (unsigned int j=0; j< number_of_int_keys(); ++j) { + ints_[i][j]= p->get_value(get_int_key(j)); } } generate_move(f); @@ -34,11 +38,11 @@ { for (unsigned int i=0; i< number_of_particles(); ++i) { Particle *p = get_particle(i); - for (unsigned int j=0; j< fkeys_.size(); ++j) { - p->set_value(fkeys_[j], floats_[i][j]); + for (unsigned int j=0; j< number_of_float_keys(); ++j) { + p->set_value(get_float_key(j), floats_[i][j]); } - for (unsigned int j=0; j< ikeys_.size(); ++j) { - p->set_value(fkeys_[j], floats_[i][j]); + for (unsigned int j=0; j< number_of_int_keys(); ++j) { + p->set_value(get_int_key(j), ints_[i][j]); } } } Index: kernel/src/optimizers/movers/NormalMover.cpp =================================================================== --- kernel/src/optimizers/movers/NormalMover.cpp (revision 569) +++ kernel/src/optimizers/movers/NormalMover.cpp (working copy) @@ -13,14 +13,12 @@ namespace IMP { -NormalMover::NormalMover(const Particles &pis, - const FloatKeys &vars, - Float max) +NormalMover::NormalMover(const FloatKeys &vars, + Float max, + const Particles &pis) { add_particles(pis); - for (unsigned int i = 0; i < vars.size(); ++i) { - add_key(vars[i]); - } + add_float_keys(vars); set_sigma(max); } Index: kernel/src/optimizers/movers/BallMover.cpp =================================================================== --- kernel/src/optimizers/movers/BallMover.cpp (revision 569) +++ kernel/src/optimizers/movers/BallMover.cpp (working copy) @@ -46,14 +46,12 @@ -BallMover::BallMover(const Particles &pis, - const FloatKeys &vars, - Float max) +BallMover::BallMover(const FloatKeys &vars, + Float max, + const Particles &pis) { add_particles(pis); - for (unsigned int i = 0; i < vars.size(); ++i) { - add_key(vars[i]); - } + add_float_keys(vars); set_radius(max); }

1 0

Re: [IMP-dev] [IMP-commits] r566 - in trunk/kernel: include/IMP include/IMP/internal pyext src src/restraints src/score_states
by Daniel Russel 19 May '08

19 May '08

> > Modified: trunk/kernel/include/IMP/internal/ParticleGrid.h > =================================================================== > --- trunk/kernel/include/IMP/internal/ParticleGrid.h 2008-05-17 > 00:35:56 UTC (rev 565) > +++ trunk/kernel/include/IMP/internal/ParticleGrid.h 2008-05-18 > 07:07:11 UTC (rev 566) > @@ -158,8 +158,8 @@ > } > bool operator==(const ParticleVoxelIterator &o) const { > return cvoxel_== o.cvoxel_ && (cvoxel_ == evoxel_ > - && o.cvoxel_ == o.evoxel_ > - || curp_ == o.curp_); > + && (o.cvoxel_ == o.evoxel_ > + || curp_ == o.curp_)); > } > bool operator!=(const ParticleVoxelIterator &o) const { > return !operator==(o); > This changes the logic and I don't think it is correct. BTW, what didn't gcc 4.3 like with the Keys? Thanks.

2 2

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/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()),

2 3