[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[IMP-dev] Removing particle attributes



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);