Just a couple of comments: - it would be really nice is the other packages used the logging support built into the IMP kernel. This makes controlling the complexity and where it goes much easier.
- is there a reason that other modules don't use the imp kernel exception wrappers? Is this something to do with SWIG failing or a design decision? It seems to me uniformity in exception returns would also be quite nice.
Notification of IMP commits wrote: > Author: ben@SALILAB.ORG > Date: 2008-09-09 19:31:05 -0700 (Tue, 09 Sep 2008) > New Revision: 715 > > Added: > trunk/domino/test/test_restraints.py > Modified: > trunk/domino/include/SimpleDiscreteRestraint.h > trunk/domino/pyext/DOMINO.i > trunk/domino/src/SimpleDiscreteRestraint.cpp > Log: > - Have SimpleDiscreteRestraint's constructor throw a proper exception on reading > an invalid format restraints file. > - Add a test case for this exception. > - Have the Python wrapper translate exceptions to Python. > > > Modified: trunk/domino/include/SimpleDiscreteRestraint.h > =================================================================== > --- trunk/domino/include/SimpleDiscreteRestraint.h 2008-09-10 01:34:59 UTC (rev 714) > +++ trunk/domino/include/SimpleDiscreteRestraint.h 2008-09-10 02:31:05 UTC (rev 715) > @@ -27,6 +27,9 @@ > class IMPDOMINOEXPORT SimpleDiscreteRestraint : public Restraint > { > public: > + //! Constructor. > + /** \exception ErrorException the restraint file is of an invalid format. > + */ > SimpleDiscreteRestraint(Model& model_, std::string restraint_filename, > Particle *p1, Particle *p2); > > > Modified: trunk/domino/pyext/DOMINO.i > =================================================================== > --- trunk/domino/pyext/DOMINO.i 2008-09-10 01:34:59 UTC (rev 714) > +++ trunk/domino/pyext/DOMINO.i 2008-09-10 02:31:05 UTC (rev 715) > @@ -11,6 +11,7 @@ > %} > > %include "kernel/pyext/IMP_macros.i" > +%include "kernel/pyext/IMP_exceptions.i" > > /* Ignore shared object import/export stuff */ > #define DOMINODLLEXPORT > @@ -28,9 +29,6 @@ > /* Get definitions of kernel base classes (but do not wrap) */ > %import "kernel/pyext/IMP.i" > > -/* Don't use the exception handlers defined in the kernel */ > -%exception; > - > /* Wrap our own classes */ > %include "IMP/domino/DiscreteSampler.h" > %include "IMP/domino/CombState.h" > > Modified: trunk/domino/src/SimpleDiscreteRestraint.cpp > =================================================================== > --- trunk/domino/src/SimpleDiscreteRestraint.cpp 2008-09-10 01:34:59 UTC (rev 714) > +++ trunk/domino/src/SimpleDiscreteRestraint.cpp 2008-09-10 02:31:05 UTC (rev 715) > @@ -34,9 +34,10 @@ > atoi(v[1].c_str()))] > = atof(v[2].c_str()); > } else { > - std::cout << "SimpleDiscreteRestraint::load_restraints the line : " > - << line << " is of the wrong format" << std::endl; > - throw(1); > + std::ostringstream msg; > + msg << "SimpleDiscreteRestraint::load_restraints the line : " > + << line << " is of the wrong format"; > + IMP_failure(msg.str().c_str(), ErrorException); > } > } > } > > Added: trunk/domino/test/test_restraints.py > =================================================================== > --- trunk/domino/test/test_restraints.py (rev 0) > +++ trunk/domino/test/test_restraints.py 2008-09-10 02:31:05 UTC (rev 715) > @@ -0,0 +1,25 @@ > +import unittest > +import os > +import IMP > +import IMP.domino > +import IMP.test > + > +class RestraintTests(IMP.test.TestCase): > + """Tests for the SimpleDiscreteRestraint""" > + > + def test_invalid_restraints_file(self): > + """Loading an invalid restraints file should give an error""" > + fname = "invalid" > + m = IMP.Model() > + p1 = IMP.Particle() > + p2 = IMP.Particle() > + m.add_particle(p1) > + m.add_particle(p2) > + > + print >> file(fname, "w"), "garbage" > + self.assertRaises(RuntimeError, IMP.domino.SimpleDiscreteRestraint, > + m, fname, p1, p2) > + os.unlink(fname) > + > +if __name__ == '__main__': > + unittest.main() > > _______________________________________________ > IMP-commits mailing list > IMP-commits@salilab.org > https://salilab.org/mailman/listinfo/imp-commits >
Daniel Russel wrote: > - it would be really nice is the other packages used the logging support > built into the IMP kernel. This makes controlling the complexity and > where it goes much easier.
Of course - I agree, and will help people like Keren to use the IMP kernel facilities where appropriate.
> - is there a reason that other modules don't use the imp kernel > exception wrappers? Is this something to do with SWIG failing or a > design decision? It seems to me uniformity in exception returns would > also be quite nice.
All modules should use the IMP kernel exception wrappers, which is why I made the change to the domino module in this commit. Perhaps the em module should too, but as far as I could tell from Keren's interface and documentation, neither of her classes throw any IMP or EMBED exceptions. Keren, perhaps you could correct me if I'm wrong here.
The reason for the explicit %exception; in the em module currently is that SWIG's %import sets up the IMP %exception handler but does not emit the static handle_imp_exception() method into the actual wrapper, so it doesn't compile. An explicit %include of IMP_exceptions is needed instead to use the kernel exception wrappers.
Ben
-
Ben Webb -
Daniel Russel