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

Avoid adjusting input entries in mixing schemes. #2830

Closed
wants to merge 4 commits into from

Conversation

peikai
Copy link
Contributor

@peikai peikai commented Feb 4, 2023

Here to submit changes to avoid change input entries in place.

In both of GGA/GGA+U and GGA/GGA+U/R2SCAN mixing processes, the methods below not only return adjusted entries, but also change input entries in place. Is it necessary? I think this may cause bugs in programming.

MaterialsProjectDFTMixingScheme.process_entries()
entries.compatibility.process_entries()

Thanks!

@coveralls
Copy link

coveralls commented Feb 4, 2023

Coverage Status

Coverage: 74.3% (-5.2%) from 79.463% when pulling 7606c36 on peikai:EntriesCopy into c6f9bba on materialsproject:master.

@peikai
Copy link
Contributor Author

peikai commented Feb 4, 2023

The usage of process_entries() method needs attention. It only returns the entries that have been processed, not always the complete set of adjusted entries. If input entries have contained energy adjustments, they would not be processed again, and even would not be shown in the results that are returned (if process_entries(clean=False)).

In the case, processed_entryList = compatibility.process_entries(entryList, clean=True), entryList will be changed as well.
In the case, processed_entryList = compatibility.process_entries(entryList, clean=False), processed_entryList may not have a complete set of entries, if there are some entries containing energy_adjustements in entryList.

I would close this PR, to avoid changing this usage.

@peikai
Copy link
Contributor Author

peikai commented Feb 12, 2023

This PR is renewed in #2841.

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

2 participants