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

Python3 safe division #274

Closed
wants to merge 5 commits into from
Closed

Conversation

siscia
Copy link
Contributor

@siscia siscia commented Sep 2, 2019

PR as discussed in #270

@siscia
Copy link
Contributor Author

siscia commented Sep 2, 2019

Honestly I didn't even run the test on this PR.

@JLTastet
Copy link
Contributor

JLTastet commented Sep 2, 2019

Nice! Note that division_safe is the same fixture as before, which uses old_div. So the division bug should still be present, but the code will be easier to review / fix now that each stage has its own commit.

@JLTastet
Copy link
Contributor

JLTastet commented Sep 2, 2019

I will try to run the tests a bit later on Stage 2 without division. If they pass, then we can add from __future__ import division just for physics models, hence defaulting to real division in both Python 2 and 3.

@JLTastet
Copy link
Contributor

JLTastet commented Sep 2, 2019

I ran the tests again without the old_div, and they all pass now. I will extend them to include the Dark Photon model, and if they still pass I will explicitly import the real division in all the relevant files and open a PR.

@siscia I think it would be a good idea to get all your commits except the Stage 2 ones merged as soon as possible (*). Then we can "futurize" and validate individual files on a case-by-case basis, until they all support Python 3. But if we futurize everything at once, I fear we will introduce more silent breakage.

(*) We also need to keep in mind that indentations changes etc. will cause a lot of conflicts when rebasing, so if anyone has a long-lived feature branch lying around, it might be a good idea to get it merged ASAP. Maybe we could make an announcement on the mailing list?

@JLTastet
Copy link
Contributor

JLTastet commented Sep 2, 2019

After a few fixes (cf. #272), dark photon tests pass too.
I will modify the relevant files tomorrow to use the real division where it makes sense.

@JLTastet
Copy link
Contributor

JLTastet commented Sep 3, 2019

#275 defaults to using real division in physics models.

@JLTastet JLTastet mentioned this pull request Sep 12, 2019
46 tasks
@JLTastet
Copy link
Contributor

@siscia Can you open a new PR with just the pickler trick? Thanks!

@siscia
Copy link
Contributor Author

siscia commented Sep 12, 2019

Please don't ask me to do it...

You can just copy the new pickler and make a PR yourself...

@JLTastet
Copy link
Contributor

Ok, I'll cherry-pick your commit

@siscia
Copy link
Contributor Author

siscia commented Sep 12, 2019

Many thanks! :)

@ThomasRuf
Copy link
Contributor

outdated

@ThomasRuf ThomasRuf closed this Jun 16, 2022
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