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

Propagate labels through various Structure operations #3183

Merged
merged 12 commits into from
Jul 31, 2023

Conversation

stefsmeets
Copy link
Contributor

Summary

This PR follows up from #3137 and makes sure that labels are propagated correctly throughout structure, for example, when making supercells, inserting/deleting sites, applying translations/rotations, etc.

This PR also fixes a bug where the labels were incorrectly assigned when the CIF contains multiple species with different names.

Major changes:

  • feature 1: Propagate labels in operations on structure
  • fix 1: Correctly generate labels when cif contains multiple species with same name

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)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

@janosh
Copy link
Member

janosh commented Jul 25, 2023

Thank you very much @stefsmeets, this is great! 🙏

Just to get an overview, are there more structure operations that are known to drop the site labels? Supercells, inserting/deleting sites, applying translations/rotations are all very important. Just want to leave a record here of what else might need doing in the future.

@stefsmeets
Copy link
Contributor Author

stefsmeets commented Jul 26, 2023

@janosh Good point, I think these functions could still be updated:

  • IStructure.from_spacegroup
  • IStructure.from_magnetic_spacegroup
  • IStructure.get_sites_in_sphere
  • IStructure.get_all_neighbors
  • IStructure.interpolate
  • IMolecule.get_sites_in_sphere
  • IMolecule.get_boxed_structure
  • IMolecule.get_centered_molecule
  • Everything to do with Neighbor/PeriodicNeighbor

I will update the PR with fixes for these today or tomorrow.

There is also the CIF writer which drops the labels. I looked at this before, but I found it a bit tricky to update.

@janosh
Copy link
Member

janosh commented Jul 26, 2023

There is also the CIF writer which drops the labels. I looked at this before, but I found it a bit tricky to update.

Feel free to just leave a todo comment for CIFParser so that we know it should still be done at some point.

@stefsmeets
Copy link
Contributor Author

stefsmeets commented Jul 27, 2023

I had a go at updating the CifWriter with labels. Since the label defaults to Site.species_string, it turns out that it's a little bit more straightforward than when I first looked at this.

I also updated site.label to be a property. This helps generate the appropriate label for methods that update Site inplace (such as SitesCollection.add_oxidation_state_by_*).

Let me know what you think.

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.

Almost ready to merge! Thanks for the great work. I think making label a @property was the right call.

pymatgen/io/cif.py Outdated Show resolved Hide resolved
pymatgen/io/tests/test_cif.py Outdated Show resolved Hide resolved
pymatgen/io/tests/test_cif.py Outdated Show resolved Hide resolved
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.

@stefsmeets Thanks a lot for this PR! You really went the extra mile.

@janosh janosh enabled auto-merge (squash) July 31, 2023 15:18
@janosh janosh merged commit f1ac4aa into materialsproject:master Jul 31, 2023
26 checks passed
@mkhorton
Copy link
Member

I noticed that performing a Structure.replace_species operation leaves the label intact, leading to a misleading label or a confusing end-user experience (for example, replacing Au with {"Au": 0.5, "Cu": 0.5} will leave the label as Au). Do you have any thoughts on this @stefsmeets?

@stefsmeets
Copy link
Contributor Author

I noticed that performing a Structure.replace_species operation leaves the label intact, leading to a misleading label or a confusing end-user experience (for example, replacing Au with {"Au": 0.5, "Cu": 0.5} will leave the label as Au). Do you have any thoughts on this @stefsmeets?

I don't have any particular thoughts on this, but I can see how it would be confusing. What would you expect to happen?

@stefsmeets stefsmeets deleted the label-update branch February 22, 2024 12:25
@janosh
Copy link
Member

janosh commented Feb 22, 2024

What would you expect to happen?

maybe update the label to Composition({"Au": 0.5, "Cu": 0.5}).formula

@stefsmeets
Copy link
Contributor Author

I can have a go at this next week. I created a new issue out of it to keep track.

@fxcoudert
Copy link

I think this caused a regression in CIF output: #3761

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.

None yet

4 participants