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 rounding during FEFF writing #3345

Merged
merged 3 commits into from Feb 26, 2024

Conversation

matthewcarbone
Copy link
Contributor

pymatgen/io/feff/inputs.py lines 504-506 rounded the FEFF geometry to 6 decimal places by default. This also occurs in 518-520. Other writers (such as the one for EXCITING) do not appear to do this. This commit makes a simple change and removes the ":f" from the string representation in the lines formatting. Thus FEFF clusters will now be saved with default precision (or whatever self._cluster uses).

I'm implementing this fix because it's actually causing me some precision issues when testing save spectroscopy input files for FEFF against other codes, and also because this would make the FEFF writers consistent.

I'm also happy to use this PR to help make the other codes consistent. Thoughts?

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Note I don't think that there are any new tests to run here.

pymatgen/io/feff/inputs.py lines 504-506 rounded the FEFF geometry
to 6 decimal places by default. This also occurs in 518-520.
Other writers (such as the one for EXCITING) do not appear to do this.
This commit makes a simple change and removes the ":f" from the string
representation in the lines formatting. Thus FEFF clusters will now
be saved with default precision (or whatever self._cluster uses).
@janosh
Copy link
Member

janosh commented Sep 21, 2023

I'm also happy to use this PR to help make the other codes consistent. Thoughts?

Thanks @matthewcarbone. Which other codes are you referring to?

@janosh janosh added io Input/output functionality fix Bug fix PRs labels Sep 21, 2023
@matthewcarbone
Copy link
Contributor Author

@janosh

Thanks @matthewcarbone. Which other codes are you referring to?

Well, selfishly the ones I use here: FEFF, EXCITING, Xspectra, OCEAN and VASP. But that can just be a starting point!

@janosh
Copy link
Member

janosh commented Sep 21, 2023

I think I can see both sides. Why not write geometries as is, as you're suggesting.

On the other side, there's usually no point in recording higher float precision than what your data has i.t.o. significant digits. Is yours more accurate than 1e-6? If not, maybe a rounded equality check or using something like np.allclose would be a better option for you?

@matthewcarbone
Copy link
Contributor Author

I think I can see both sides. Why not write geometries as is, as you're suggesting.

It's more just write them at the precision specified by self._cluster instead of constraining the precision in a way that seems arbitrary.

On the other side, there's usually no point in recording higher float precision than what your data has i.t.o. significant digits. Is yours more accurate than 1e-6? If not, maybe a rounded equality check or using something like np.allclose would be a better option for you?

Certainly true. This is really a nitpick, but when doing some sanity checks on my end, the problem is that the precision differences cause sorting issues which scramble the order of atoms! I can give you the long story "long", if you want, but that's the long story short.

Is it a big deal? No absolutely not. But why not be consistent!

@janosh
Copy link
Member

janosh commented Sep 22, 2023

But why not be consistent!

Sure, I'm happy to go ahead with this. I'm also not a user of these parts of pymatgen myself so just trying to get an idea on what's the best course of action. The other option that comes to mind is to add an explicit fmt: str = '' keyword. So the old behavior could still be retained when users pass fmt=".f".

@matthewcarbone
Copy link
Contributor Author

Sure, I'm happy to go ahead with this. I'm also not a user of these parts of pymatgen myself so just trying to get an idea on what's the best course of action. The other option that comes to mind is to add an explicit fmt: str = '' keyword. So the old behavior could still be retained when users pass fmt=".f".

Yes we definitely could do that too! What would you like to do? If you want I can implement this. We'd have to have a lot of passthroughs from the FEFFDictSet -> Atoms -> etc. so it will take another commit or two.

@janosh
Copy link
Member

janosh commented Sep 22, 2023

Please go ahead.

@matthewcarbone matthewcarbone marked this pull request as draft September 22, 2023 12:56
@janosh janosh added the stale Abandoned or conflicting PRs and outdated issues label Oct 8, 2023
@janosh janosh force-pushed the master branch 2 times, most recently from 3c23114 to 36e289c Compare December 19, 2023 02:10
@janosh janosh force-pushed the master branch 4 times, most recently from d725325 to dca98be Compare February 2, 2024 11:47
@janosh janosh marked this pull request as ready for review February 26, 2024 08:54
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

merging as is now since it went stale and seems like a small improvement

@janosh janosh enabled auto-merge (squash) February 26, 2024 08:55
@janosh janosh merged commit 4179377 into materialsproject:master Feb 26, 2024
22 checks passed
@matthewcarbone matthewcarbone deleted the mc/fix-feff-precision branch March 3, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs io Input/output functionality stale Abandoned or conflicting PRs and outdated issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants