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

Make run_type in mixing scheme consistent with entries #4255

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

peikai
Copy link
Contributor

@peikai peikai commented Jan 16, 2025

The run_type for entries had been changed from 'R2SCAN' to 'r2SCAN', whereas the MaterialsProjectDFTMixingScheme() is still expecting to find 'R2SCAN' entries.

As a result, all r2SCAN entries would be ignored when carrying out GGA(+U)/R2SCAN mixing scheme corrections, thereby affecting the construction of GGA(+U)/R2SCAN mixing phase diagram 1.

Error message would be:

pymatgen\entries\mixing_scheme.py:176: UserWarning: Invalid run_type='r2SCAN' for entry mp-1018134-r2SCAN. Must be one of ['GGA', 'GGA+U', 'R2SCAN']. This entry will be ignored.

Footnotes

  1. https://docs.materialsproject.org/methodology/materials-methodology/thermodynamic-stability/phase-diagrams-pds#gga-ggau-r2scan

@mkhorton
Copy link
Member

Good catch, thanks @peikai!

If you could, can you change the check to ensure it is not case sensitive? This is probably the most robust solution, in case some people are still using entries serialised with the former run_type.

@shyuep shyuep enabled auto-merge (squash) January 17, 2025 16:54
auto-merge was automatically disabled January 17, 2025 18:11

Head branch was pushed to by a user without write access

@peikai
Copy link
Contributor Author

peikai commented Jan 17, 2025

@mkhorton, Sure, I updated the calculation parameters for archive test entries, changing 'R2SCAN' to 'r2SCAN' in parameters['run_type']. Then, added an unittest to check run types in mixing scheme are consistent to calculation parameters.

In addition, comments that use R2SCAN as a run type are also revised, according to a principle: the run type that inherits from calculation parameters of VASP should use 'r2SCAN' to keep consistent with the parameter dictionary in entry object; when talking about the thermo type, 'R2SCAN' is still retained to divide problems considering it has been a frequently used glossary. However, I noticed that the wording for this thermo type has been changing to r2SCAN. 1

Also, I would like to mention that there are two run_type terms in an entry object, located in the parameter and the data dictionary respectively. For an r2SCAN entry, the run_type in parameter dict is 'r2SCAN', whereas the run_type in data dict is 'R2SCAN', which still easy to make confusion.

Footnotes

  1. https://docs.materialsproject.org/changes/database-versions#v2024.12.18:~:text=GGA_GGA%2BU_R2SCAN%3E-,r2SCAN,-%3E%20GGA_GGA%2BU

@esoteric-ephemera
Copy link
Contributor

Hey @peikai, we recently changed the run_type values used by the Materials Project in the emmet package to account for how DFT functionals are written in literature. So MP's data now uses r2SCAN as the run_type rather than R2SCAN. Similarly, other data now has PBEsol as the run_type

There's a mismatch between what the pymatgen corrections/phase diagram class expects and what MP data returns. In the long run, might be better to implement @mkhorton's suggestion since there shouldn't be functionals with names that differ only in capitalization. Happy to do so

@peikai
Copy link
Contributor Author

peikai commented Jan 18, 2025

Hey @esoteric-ephemera, it is great that glossary is consistent to calculation. Also, the run_type in data dictionary might need update, for all entries, or link them as one directly. The commits above had incorporated an unittest for run_type consistency during mixing scheme correcting.

@peikai
Copy link
Contributor Author

peikai commented Jan 19, 2025

@mkhorton Oh, I misunderstood the word case-sensitive. Here are updated commits to permit both 'r2SCAN' entries and archived 'R2SCAN' entries for mixing scheme correction, which would fix that 'r2SCAN' entries were ignored in mixing scheme correction. Furthermore, 'r2SCAN' as an input run_type would not be case-sensitive, to give backward compatibility, ensuring former default input, 'R2SCAN', can still work in the mixing scheme. The extendibility for possible run_type variations is taken into account. A unit test has been added to check it worked properly. And the name of input run_type of MaterialsProjectDFTMixingScheme() moves forward as the 'r2SCAN'.

Any modifications are welcome, it would be better if there is an implement realizing case insensitive of the run_type only in test files. Alternatively, it is a chance to clear and update archive R2SCAN entries in test files.

@peikai peikai reopened this Jan 20, 2025
@shyuep shyuep merged commit 0dcbe0e into materialsproject:master Jan 23, 2025
4 of 5 checks passed
@mkhorton
Copy link
Member

Many thanks Kai!

@peikai
Copy link
Contributor Author

peikai commented Jan 28, 2025

No problem! Thanks for the instructions.

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.

4 participants