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

remove -ffast-math compiler flag #4220

Merged
merged 2 commits into from
Aug 2, 2023
Merged

remove -ffast-math compiler flag #4220

merged 2 commits into from
Aug 2, 2023

Conversation

orbeckst
Copy link
Member

@orbeckst orbeckst commented Aug 1, 2023

Fixes #4211 and #3821

Changes made in this Pull Request:

  • removed -ffast-math from the compiler flags in setup.py
  • Without -ffast-math, the AtomGroup.center_of_charge(..., unwrap=True) pass again on Intel x86_64 macOS.

PR Checklist

  • Tests? (existing, checked locally on Intel mac 15.2)
  • n/a Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

Developers certificate of origin


📚 Documentation preview 📚: https://mdanalysis--4220.org.readthedocs.build/en/4220/

- fix #4211
- fix #3821
- Without -ffast-math, the AtomGroup.center_of_charge(..., unwrap=True) pass again
  on Intel x86_64 macOS.
- updated CHANGELOG
@orbeckst orbeckst linked an issue Aug 1, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Linter Bot Results:

Hi @orbeckst! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (66a2b21) 93.62% compared to head (0b9b2d7) 93.62%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4220   +/-   ##
========================================
  Coverage    93.62%   93.62%           
========================================
  Files          193      193           
  Lines        25295    25295           
  Branches      4063     4063           
========================================
  Hits         23683    23683           
  Misses        1096     1096           
  Partials       516      516           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Makes sense, it's probably going to have a performance impact but we've been meaning to get rid of that flag anyways.

Do we want to backport this into a 2.5.1? If so could you the move the changelog entry under a 2.5.1 and I'll make a release.

Comment on lines 24 to 26
* Fix AtomGroup.center_of_charge(..., unwrap=True) giving wrong
results on Intel macOS by removing `-ffast-math` compiler flag
(Issues #3821, #4211)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how we deal with this stuff, but is it worth adding an entry under "Changed" too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure, especially now considering that @hmacdope said in #4211 (comment) that the failing test is not scientifically wrong (under PBC), just technically wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated CHANGELOG and also included @hmacdope suggestion to include the URL of the original blog post. That's a bit unusual for a CHANGELOG but in this case it's so esoteric that it seems justified and easier than having to comb through the linked issue.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Could also note that we are not infectious any more https://moyix.blogspot.com/2022/09/someones-been-messing-with-my-subnormals.html

@orbeckst
Copy link
Member Author

orbeckst commented Aug 2, 2023

Please let me know with "changes requested" what you need me to do. Sorry, I have a ton of other things to attend to and it's a lot easier just to be told what to fix than having to think hard what ought to be done. Thanks!

@orbeckst
Copy link
Member Author

orbeckst commented Aug 2, 2023

Re backport #4220 (review) : given @hmacdope 's assessment that the result of the test is still scientifically valid, I don't think we need a backport as I would consider it a small reproducibility issue but not a correctness issue and we (=you @IAlibay ) can spend time on other things. But if anyone else thinks we should backport, I am of course happy to change the CHANGELOG.

@IAlibay
Copy link
Member

IAlibay commented Aug 2, 2023

I don't think we need a backport as I would consider it a small reproducibility issue but not a correctness issue

Sounds good, I think we're likely close to needing another release soon (we're at 2.2 months roughly and we're meant to have a 3 monthly cadence) so we could just say we'll release at some point in August.

@orbeckst
Copy link
Member Author

orbeckst commented Aug 2, 2023

@RMeli can I please ask you to shepherd the PR to completion?

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

Thanks @orbeckst, LGTM

@RMeli RMeli merged commit 2bf55dd into develop Aug 2, 2023
@RMeli RMeli deleted the remove-fast-math branch August 16, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.5.0] Center of charge test failures on macOS runners Remove --fast-math from compile flags?
4 participants