binder
binder copied to clipboard
Pre post increment ops renamed
This is a feature add that also fixes the same bugs that https://github.com/RosettaCommons/binder/pull/237 fixes (as the code introduced in https://github.com/RosettaCommons/binder/pull/237 made it easy to add this feature). It addresses https://github.com/RosettaCommons/binder/issues/226
As mentioned in https://github.com/RosettaCommons/binder/pull/237 there are a few possible options we can do:
- Close this and post/pre increment both named
plus_plus
, with post requiring a integer dummy argument (This makes it harder to tell++a
froma++
apart) - Merge this and rename define
pre_increment()
andpost_increment(int)
(This is not backwards compatible) - Improve this and define
pre_increment()
andpost_increment()
(This would remove the ability to control that dummy integer in the rare case someone decided to use it for some reason) - Do either 2 or 3, but also keep
plus_plus
for compatibility (This might add overhead) - Add a CLI flag to decide between options
Should I do anything to get this PR moving?
@lyskov @zwimer FWIW, renaming "plus_plus" to "pre_increment" and "post_increment" is a breaking change to a very large amount of the PyRosetta code that interfaced classes with these two operators (E.g. anything that looks at the EnergyGraph, the TenANeighborGraph, the ConstraintGraph, or just Graph). Now if a piece of PyRosetta code straddles two boxes with different versions of PyRosetta installed on them, that code cannot run on both machines. Is it possible to have both "pre_increment" and "plus_plus" map to the same function?
@aleaverfay Hi Andrew! Sorry to hear that these changes broke your code! These change have not break any of PyRosetta unit tests (and I have not heard so far from other users regarding this change) so I am guessing that it could be something specific to area of code you are working with. Never the less let's see if we can find a good solution that works for you!
I am a bit skeptical about "double bindings" since it will just prolong the issue (and then we will have one more API function to take care of) (and currently Binder does not have such functionality). The code that got broken, - is that something that you will be OK/easy/have-permission to modify? If so, have you considered the following workaround which should have ~zero performance impact:
During import phase, we can check if particular class already have plus_plus
defined and if not just link its implementation to pre_increment
/post_increment
, like this:
...
import aaaa
if not hasattr(aaaa.A, 'plus_plus'): aaaa.A.plus_plus = aaaa.A.pre_increment
-- will something like this work for you? Thanks,