Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre post increment ops renamed #238

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

zwimer
Copy link
Contributor

@zwimer zwimer commented Oct 12, 2022

This is a feature add that also fixes the same bugs that #237 fixes (as the code introduced in #237 made it easy to add this feature). It addresses #226

@zwimer
Copy link
Contributor Author

zwimer commented Oct 13, 2022

As mentioned in #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
Copy link
Contributor Author

zwimer commented Dec 13, 2022

Should I do anything to get this PR moving?

Copy link
Member

@lyskov lyskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if my previous comment on was a bit cryptic. The main issue with merging this is backward compatibility with already generate code. However i just checked and it looks like there is only a few cases where these was uses in my project so i am open on merging this. Please see minor comment below that needs to be address before this is merged. Thanks,

source/function.cpp Outdated Show resolved Hide resolved
Copy link
Member

@lyskov lyskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, - thank you for putting this together @zwimer !

@lyskov lyskov merged commit c51ec70 into RosettaCommons:master Dec 15, 2022
@aleaverfay
Copy link
Member

aleaverfay commented Dec 8, 2023

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

@lyskov
Copy link
Member

lyskov commented Dec 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants