binder icon indicating copy to clipboard operation
binder copied to clipboard

Pre post increment ops renamed

Open zwimer opened this issue 2 years ago • 1 comments

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

zwimer avatar Oct 12 '22 00:10 zwimer

As mentioned in https://github.com/RosettaCommons/binder/pull/237 there are a few possible options we can do:

  1. Close this and post/pre increment both named plus_plus, with post requiring a integer dummy argument (This makes it harder to tell ++a from a++ apart)
  2. Merge this and rename define pre_increment() and post_increment(int) (This is not backwards compatible)
  3. Improve this and define pre_increment() and post_increment() (This would remove the ability to control that dummy integer in the rare case someone decided to use it for some reason)
  4. Do either 2 or 3, but also keep plus_plus for compatibility (This might add overhead)
  5. Add a CLI flag to decide between options

zwimer avatar Oct 13 '22 00:10 zwimer

Should I do anything to get this PR moving?

zwimer avatar Dec 13 '22 04:12 zwimer

@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 avatar Dec 08 '23 14:12 aleaverfay

@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,

lyskov avatar Dec 08 '23 21:12 lyskov