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

Raise ValueError when setting chem potentials with mixed type but species duplicates #114

Merged
merged 3 commits into from
Feb 8, 2021

Conversation

lbluque
Copy link
Collaborator

@lbluque lbluque commented Feb 5, 2021

Summary

In #113 @juliayang pointed out that setting the chemical potentials of the vacancies with a string as a key was not changing the values being sampled in the ensemble.
This was indeed a bug that happens when the chemical potential dictionary being used has a species duplicate from using mixed types as keys. In the example notebook the duplicate was having a Vacancy object and a "Vacancy" string as different keys in the dictionary, thus when updating the value for "Vacancy" it was being ignored since only the value for Vacancy was being used (or viceversa, whichever comes up last in the dict.items())

This PR simply raises an error when it finds duplicate species in the dictionary being used to set the values of chemical potentials or fugacity fractions.

Additional dependencies introduced (if any)

TODO (if any)

Checklist

@lbluque lbluque requested a review from juliayang February 5, 2021 23:26
@lbluque
Copy link
Collaborator Author

lbluque commented Feb 5, 2021

@juliayang can you have a quick look at this. The only pertinent file should be smol/moca/ensemble/semigrand.py in the chemical_potential.setter

Copy link
Contributor

@juliayang juliayang left a comment

Choose a reason for hiding this comment

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

Good addition @lbluque! It's working as expected.

@lbluque lbluque merged commit 8f0352a into CederGroupHub:master Feb 8, 2021
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.

2 participants