Skip to content

Bug fix: change CifWriter to save only the element symbol, and not the entire species string#3071

Open
kamronald wants to merge 3 commits intomaterialsproject:masterfrom
kamronald:fix_cif_writer
Open

Bug fix: change CifWriter to save only the element symbol, and not the entire species string#3071
kamronald wants to merge 3 commits intomaterialsproject:masterfrom
kamronald:fix_cif_writer

Conversation

@kamronald
Copy link

Change the "atom site type symbol", such that the element symbol is saved instead of the full species string (which includes site properties). This fixes Issue #3065, allowing for accurate writing of structures containing species properties to CIF and MCIF files.

Summary

Major updates:

Change the "atom site type symbol", such that the element symbol is saved instead of the full species string (which includes site properties).

Todos

Checklist

Ensure:

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • All tests and linting pass.

Change the atom site type symbol, such that the element symbol is saved instead of the full species string (which includes site properties)
@mkhorton
Copy link
Member

@kamronald does this still handle species with oxidation state correctly?

@kamronald
Copy link
Author

@mkhorton good point, I just checked and it does not handle them properly, I'll fix that

atom_site_type_symbol now saves the element and oxidation state, without the other site properties
@kamronald
Copy link
Author

@mkhorton oxidation states are now properly saved

@janosh
Copy link
Member

janosh commented Jun 16, 2023

Thanks @kamronald! 👍

IIUC @mkhorton in #3065 (comment), he's suggesting we continue saving scalar magmoms to regular CIF using the local data name mp registered here https://www.iucr.org/cgi-bin/cifreserve.pl.

Also, would be great if you could refactor your repro #3065 (comment) into a new test case.

@janosh janosh added needs testing PRs that are not ready to merge due to lacking tests io Input/output functionality fix Bug fix PRs labels Jun 16, 2023
@kamronald
Copy link
Author

@janosh @mkhorton Yes will work on that

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.09%. Comparing base (a1c19db) to head (324a675).
Report is 1345 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3071      +/-   ##
==========================================
- Coverage   74.65%   74.09%   -0.57%     
==========================================
  Files         230      230              
  Lines       69377    69382       +5     
  Branches    16154    16154              
==========================================
- Hits        51796    51410     -386     
- Misses      14513    14937     +424     
+ Partials     3068     3035      -33     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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 force-pushed the master branch 2 times, most recently from e3fbc67 to 41e6d99 Compare August 3, 2024 19:01
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 needs testing PRs that are not ready to merge due to lacking tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants