> Maybe your case should be an exception, but in general I completely > agree with Ben's policy. > Ben's policy assures that IMP is basically working. Ben's policy does nothing useful towards that and in fact greatly increases the time it takes for certain bugs to get fixed. > We can not trust changes made by anyone and even in big software > companies ( such as google), every change should go through a code > review process. First of all, a large working code base should be treated differently from research code that is still young. But yes, code reviews are good.

However, no one is actually reviewing any of the code that is being submitted now. Ben spends time on pointless formatting issues, but I can count the number of bugs he has found from actually looking at the code on one finger :-) (more or less)

Further, I have accidentally committed a number of things that would have been caught by anyone paying attention to the code (I saw them as soon as I looked again). In addition, a significant fraction of the issues that we encounter are things that have been created in the process of preparing patches since that is so much more complicated than it has to be.

> IMP is being used by various people, some without formal CS background > and some are more flexible and tolerant than others. Sure, but there is no reason for them to be accessing the main svn directly. SVN is for people who want to develop IMP. That is why projects have point releases. > Just an example, today Frido and I, for our 26S project, updated IMP > and than it took us at least an hour to solve all of the broken > interfaces. This happens a lot and some people might give up on IMP or > just use an old version and never update ( Frank's MODELLER 3 in > restrainer is an extreme case for that). Yes, IMP is still changing, sometimes too much. However, that has nothing to do with the question at hand. Code review just perhaps prevents accidental breakages, those changes were all intentional and reviewed. > Code will move from the nightly to the weekly only after testcases > have been written and passed a code review process. > what do you think? That is what I proposed :-)

Good cartoon.

Keren Lasker wrote: > ok - so I am using the latest IMP version, which broke my c++ code as > now I should use get_interacting_particles., which is fine. > The problem is that it is not working for ConnectivityRestraint from c+ > +. > The first Particles I get in the ParticlesList is garbage.

What sort of garbage? Invalid pointers, or something else? Can you post your code?

> does it suppose to work on or at this point ?

Sure, it's supposed to work - at least in the revision at https://salilab.org/internal/imp/tests.html since that's from the nightly build, and all the tests passed. There are unit tests for this method. What you should be getting for ConnectivityRestraint is the default implementation from Restraint, which is a 1-element ParticlesList containing a Particles vector with all Particles in the restraint.

Ben

Ummm, same problem as last time on the first one :-)

Keren Lasker wrote: > I am using the latest version, updated an hour ago. > The code is: > Particles r_particles; > for(ParticlesList::iterator it1 = > r.get_interacting_particles().begin(); > it1 != r.get_interacting_particles().end(); it1++){ > std::cout<<it1->size()<std::endl; for(Particles::iterator it2 = it1->begin(); it2 != it1->end(); > it2++) { > r_particles.push_back(*it2); > } > } > > > The size I get from the first one is: 17507811567898 > The restraint is of type > connectivity restraint (active): > > I solved it by using: > Particles r_particles; > ParticlesList pl; > for(ParticlesList::iterator it1 = pl.begin(); > it1 != pl.end(); it1++){ > std::cout<<it1->size()<std::endl; for(Particles::iterator it2 = it1->begin(); it2 != it1->end(); > it2++) { > r_particles.push_back(*it2); > } > } > > > but I think that the first version should work as well - no ?? > > On Aug 21, 2008, at 12:54 AM, Ben Webb wrote: > > >> Keren Lasker wrote: >> >>> ok - so I am using the latest IMP version, which broke my c++ code as >>> now I should use get_interacting_particles., which is fine. >>> The problem is that it is not working for ConnectivityRestraint >>> from c+ >>> +. >>> The first Particles I get in the ParticlesList is garbage. >>> >> What sort of garbage? Invalid pointers, or something else? Can you >> post >> your code? >> >> >>> does it suppose to work on or at this point ? >>> >> Sure, it's supposed to work - at least in the revision at >> https://salilab.org/internal/imp/tests.html >> since that's from the nightly build, and all the tests passed. There >> are >> unit tests for this method. What you should be getting for >> ConnectivityRestraint is the default implementation from Restraint, >> which is a 1-element ParticlesList containing a Particles vector with >> all Particles in the restraint. >> >> Ben >> -- >> ben@salilab.org http://salilab.org/~ben/ >> "It is a capital mistake to theorize before one has data." >> - Sir Arthur Conan Doyle >> _______________________________________________ >> IMP-dev mailing list >> IMP-dev@salilab.org >> https://salilab.org/mailman/listinfo/imp-dev >> > > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev >

ok thanks !!! On Aug 21, 2008, at 1:12 AM, Ben Webb wrote:

> Keren Lasker wrote: >> I am using the latest version, updated an hour ago. >> The code is: >> Particles r_particles; >> for(ParticlesList::iterator it1 = >> r.get_interacting_particles().begin(); >> it1 != r.get_interacting_particles().end(); it1++){ >> std::cout<<it1->size()<std::endl; > for(Particles::iterator it2 = it1->begin(); it2 != it1->end(); >> it2++) { >> r_particles.push_back(*it2); >> } >> } > > Since get_interacting_particles() returns a new vector, you're > trying to > iterate from the start of one list to the end of an entirely different > one. Like Daniel says, same problem as last time. Your second code > snippet uses only one ParticlesList though (pl) so it should work > fine - > assuming you actually initialize it to something useful somewhere. > > Ben > -- > ben@salilab.org http://salilab.org/~ben/ > "It is a capital mistake to theorize before one has data." > - Sir Arthur Conan Doyle > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev

First some summary comments: - I am glad to see people actually posting to imp-dev :-)

- it seems like the two main directions to go are either - separate modules for things. If we go that route, Ben should remove the decorators, restraints and score states etc. from IMP svn and I'll package things up nicely in my svn repository. While it is not my favorite solution, it would be better than the current situation as the kernel would then be very compact and it will be easy to make patches for it.

- more open svn where people have to learn how to play nicely with one another :-)

> for example, non-functional svn versions will annoy everybody > and IMP would die rather quickly. Yes, if it is non-functional frequently. I don't see that is likely under any plan. In my experience at various companies, everyone breaks svn once, gets bitched at and then knows to be careful for the rest of their lives. Part of the training process we are supposed to be undergoing :-)

Your usage seems to be typical so far: you update svn once every few weeks. As a result, it make no difference to you if svn does not compile for 5 minutes while some is committing patches.

> if IMP is meant to be employed by outside users, > there needs to be a single responsible person whom you bug with emails > etc if something goes wrong. that person obviously needs to know the > code... This I disagree with. We are much better off with a set of people all of whom know what is going on and feel responsible. With a single person in charge other people don't feel they have to pay attention and zone out until their they update svn and their stuff breaks from issues that were posted months before.

In addition, we have serious problems if the person in charge leaves or their interests shift as often happens in academics.

> > so what speaks against the solution to put many functions into modules > where the authorship and responsibility is clear? It doesn't add any benefit over having just having one open svn repository - svn tags each file with the name of the last person who committed so you always know who made changes and so who is responsible. It is also customary to label files with the primary author in the comments at the start if that is appropriate. - separate modules means you have to remember more information for each restraint you use to know where the include is located or which python module to find it in - separate modules results in less uniform interfaces since people don't feel as obliged to stick to relevant standards (this is not necessarily the case, but seems to be true in the projects I have worked on)

> for example, > specific experimental restraints can go in there. with embed that > works nicely. anyways, some people might prefer different solutions > for specific experimental restraints and keeping it modular allows for > that ;) Nothing prevents people from keeping experimental things outside. I have lots of code that is separate from the IMP core. What we are talking about is things that are believed to be of common interest. And the more shared and easily accessible code, the easier it is for all of us.

> also different decorators and representations could go into > modules. concerning the kernel, we all hope that it will be stable one > day and API-changes are not very frequent. It will stabilize (and has stabilized). But ultimately, it doesn't matter where the code that changed in is if you use it-- your code has to change either way.

OK, then I'll work on moving the restraints and stuff I maintain into my svn repository. I'll put the general purpose restraints in IMP::dsr (unless someone has a better name to propose) and the npc dynamics ones into IMP::npc_dynamics.

How do the python library names work for sub-libraries? i.e. IMP is _IMP.so so what is IMP.em? Does it just go in a dir called IMP in your python path?

So then we will have a kernel which is actually a kernel (plus a couple random restraints and things :-)

Ben Webb wrote: > Friedrich Foerster wrote: > >> what is all the fuzz about? i thought ben already proposed a neat >> solution in that not everything needs to be in the kernel. anyway, imp >> was meant to be modular, i thought. so whatever does not need to be in >> the kernel should not be there and in a different module instead. in >> that module everybody can mess around as he wishes, at least to some >> extent. >> > > Indeed. This is exactly what I proposed, but perhaps more succinctly > put. All sorts of things rely on the kernel interfaces being stable, and > the lab culture is to use current SVN for everything, so code changes > there should go through code review, which thus far means me. The kernel > was always envisioned to be compact; almost everything else belongs in a > module. (If nothing else, this prevents the SWIG wrappers from becoming > monstrous.) This could be remotely hosted (e.g. a lab outside of UCSF, > or in a different SVN repository in the lab, as is currently done for > Assembler) or it could be in IMP SVN (as is done for domino and impEM). > Modules need not be subject to the same scrutiny - for example, Keren > has write access to the domino and impEM modules. She does not need to > send me patches, chat on the mailing list, undergo code review, etc. - > although typically she is responsible and sends any changes by me first > for review. Each module has an owner (Keren in this case) who works with > me to set whatever policies they think are sensible. For impEM, so far > this has meant I continue to review changes, since Keren was happy with > that. For Assembler, Keren and Frido have essentially equal access. One > could also envision a restrainer module owned by Jeremy, or an > npc_dynamics module owned by Daniel, for example. > > For experimental stuff that doesn't neatly fit into an existing module, > I could certainly create an 'unstable' module if there's interest. > Essentially everybody would have write access to this module. Code > review and test cases are still strongly recommended, of course, but > failures of the test cases would not result in the nightly builds > failing. So users would use the 'unstable' module at their own risk. > > As per Keren's suggestion, stuff from 'unstable' that users start to > rely on can be moved to a more specialized module, or to the kernel > after code review. (One could even imagine having both an unstable and a > stable form of a given feature, although I'd like to minimize that if > possible.) > > To avoid confusion and namespace clashes, I propose that a module 'foo' > has its C++ code live in the IMP::foo namespace, C++ headers in IMP/foo/ > (or IMP/foo.h if the module provides a convenience 'everything' header), > and Python classes in the IMP.foo module. So the existing IMPEM Python > module would become IMP.em and move from the IMP namespace to IMP::em. > This also fits in nicely with existing Python-only modules such as > IMP.modeller_intf (ideally would be IMP.modeller, but until we can > require Python 3.0 or later, we're stuck with relative imports). And if > users wonder why their code keeps breaking, it should be obvious from > looking at either C++ or Python code whether they're using 'unstable'. > > Ben >

Ben Webb wrote: > Daniel Russel wrote: > > > If you feel others are going to be using this stuff, it's probably > better in the long term to keep it in the main IMP repository, just like > domino and impEM. I think people are already using the Nonbonded lists and stuff. Definitely easier if everything is in the same place. > Trivial that way to keep it in the nightly builds (so > that you can see when the tests fail, for example). And moving a file > between modules in the same repository preserves history. Access rights > would be the same of course - only downside (depending on your > perspective) is that the SVN server enforces code standards. > As long as I can check things in, I don't really care where it is. I also would like to be able to add tests to scons for external libraries.

> >> How do the python library names work for sub-libraries? i.e. IMP is >> _IMP.so so what is IMP.em? Does it just go in a dir called IMP in your >> python path? >> > > It'll just be called _em.so, since that's the name of the module. The > build system would be responsible for installing that into the IMP > directory. > So you propose that when impEM is built, the _em.so gets copied to the kernel/pyext/IMP dir? If we are copying things around like that, why not have central place in svn for all the libraries and pyexts to go so that the impX.sh scripts don't have to be so complicated.

Ben Webb wrote: > Daniel Russel wrote: > >> As long as I can check things in, I don't really care where it is. I >> also would like to be able to add tests to scons for external libraries. >> > > Sure, you can put whatever you like in your SConscripts. > OK. Then can you make a dsr dir for me in IMP svn? Thanks.

> >> So you propose that when impEM is built, the _em.so gets copied to the >> kernel/pyext/IMP dir? If we are copying things around like that, why not >> have central place in svn for all the libraries and pyexts to go so that >> the impX.sh scripts don't have to be so complicated. >> > > That is exactly what I am doing now. > Cool.

It would be more consistent to name the EM stuff "EM" (uppercase).

Ben Webb wrote: > Daniel Russel wrote: > >> OK, then I'll work on moving the restraints and stuff I maintain into my >> svn repository. I'll put the general purpose restraints in IMP::dsr >> (unless someone has a better name to propose) and the npc dynamics ones >> into IMP::npc_dynamics. >> > > If you feel others are going to be using this stuff, it's probably > better in the long term to keep it in the main IMP repository, just like > domino and impEM. Trivial that way to keep it in the nightly builds (so > that you can see when the tests fail, for example). And moving a file > between modules in the same repository preserves history. Access rights > would be the same of course - only downside (depending on your > perspective) is that the SVN server enforces code standards. > > If you make a list of the things you want moved, I can do that for you. > (Or, I can just make an empty module for you, and you can 'svn cp' the > files into there. Then I'll 'svn rm' them from the kernel.) Plus that > way if anybody does use them, they can argue to keep them in the kernel, > or at least will be notified that things are changing. > > >> How do the python library names work for sub-libraries? i.e. IMP is >> _IMP.so so what is IMP.em? Does it just go in a dir called IMP in your >> python path? >> > > It'll just be called _em.so, since that's the name of the module. The > build system would be responsible for installing that into the IMP > directory. > > Ben >