lab-cosmo / i-pi-dev_archive

Development version of i-PI
21 stars 12 forks source link

Move all setting of dependencies to the new format #209

Closed tomspur closed 6 years ago

tomspur commented 6 years ago

This commit moves all settings of the dependencies (with dset) to the new format. This is one part for using the new dependency mechanism in issue #116.

There are still some/many usages of dset, which I'm unsure how to fix properly.

This change has been tested with the lj example, but some more smoke testing is welcome

ceriottm commented 6 years ago

Would be good to also remove the dget in this same PR. E.g. dget(obj,"key") should be dd(obj).key

tomspur commented 6 years ago

@ceriottm Thanks for the pointer. dget and dget are now fully removed. Unfortunately, there is another error popping up now, when running the harmonic example. Can this be a bug in the depend machinery?

Traceback (most recent call last):
  File "/home/tspura/sci-chem/i-pi-dev.git/bin/i-pi", line 101, in <module>
    main(args[0], options.do_yappi)
  File "/home/tspura/sci-chem/i-pi-dev.git/bin/i-pi", line 48, in main
    simulation = Simulation.load_from_xml(fn_input, request_banner=True)
  File "/home/tspura/sci-chem/i-pi-dev.git/ipi/engine/simulation.py", line 91, in load_from_xml
    simulation.bind()
  File "/home/tspura/sci-chem/i-pi-dev.git/ipi/engine/simulation.py", line 160, in bind
    s.bind(self)
  File "/home/tspura/sci-chem/i-pi-dev.git/ipi/engine/system.py", line 104, in bind
    self.motion.bind(self.ensemble, self.beads, self.nm, self.cell, self.forces, self.prng)
  File "/home/tspura/sci-chem/i-pi-dev.git/ipi/engine/motion/dynamics.py", line 165, in bind
    self.ensemble.add_econs(dself.barostat.ebaro)
  File "/home/tspura/sci-chem/i-pi-dev.git/ipi/engine/ensembles.py", line 103, in add_econs
    dd(self).econs.add_dependency(e)
  File "/home/tspura/sci-chem/i-pi-dev.git/ipi/utils/depend.py", line 199, in add_dependency
    newdep._dependants.append(weakref.ref(self))
AttributeError: 'float' object has no attribute '_dependants'
^Cmake: *** [Makefile:21: harmonic] Interrupt
ceriottm commented 6 years ago

Yeah you misunderstood how dd works. dd gets a dobject (that overrides get - like functions to return the value when you access a depend* member) and returns a view of it as a normal object so you can access the actual depend* member rather than its value. Actually if you feel inclined it would be fantastic to have a minimal working example of the depend machinery as part of the unit tests or the docs, and /or to have better source documentation in depend.py.

tomspur commented 6 years ago

Thanks for the bugfix. This was a typo on my side, as I did it right to other places too...

I have indeed some more questions about the inner workings of the depend machinery, e.g. if it is possible to add multiple different synchos to the same object. My intuitive guess would be no, which would make it possible to add some asserts of those assumptions to further test the depend machinery. I just added this one as the first one.

ceriottm commented 6 years ago

I think it does not make sense to add multiple synchros, but you can have complex synchros that work with more than two different representations. That bit of the code is the most obscure - perhaps it would make sense to revisit it. The idea is basically how to deal with quantities that can be represented in different equivalent ways - think orbitals that might be represented in different basis, etc. Perhaps not worth our time in retrospect

tomspur commented 6 years ago

The tests from ipi/tests/test_depend.py look fine as a first smoke test of the depend machinery and all pass. (Although the last one (test_readonly) seems to not test anything currently)

Are the tests also passing for you? It seems I'm having problems with many other tests recently...

tomspur commented 6 years ago

I'd say this branch is ready for review to move all to the new dependency mechanism and I'd focus on some general testing, e.g. having a closer look to the testsuite

tomspur commented 6 years ago

Rebased against master so merging is possible again

tomspur commented 6 years ago

Rebased again due to autopep8 was applied on master.

@ceriottm Can this be also merged to remove the old depend mechanism?

ceriottm commented 6 years ago

Almost there. Can you also remove deppipe (using dpipe instead) and depcopy (using dcopy) and rename for consistency depstrip to dstrip? Then I think we're really good to merge

tomspur commented 6 years ago

@ceriottm deppipe and depcopy are removed and depstrip renamed in the last two commits

ceriottm commented 6 years ago

Let's make it a 2017 merge. Thanks @tomspur for the good work and I hope this will go down smoothly with the other branches