-
Notifications
You must be signed in to change notification settings - Fork 108
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
FIX: Canonicalize coordination number sorting in DAT reader #416
Merged
bocklund
merged 5 commits into
pycalphad:develop
from
jorgepazsoldanpalma:Z_value_correction
May 19, 2022
Merged
FIX: Canonicalize coordination number sorting in DAT reader #416
bocklund
merged 5 commits into
pycalphad:develop
from
jorgepazsoldanpalma:Z_value_correction
May 19, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #416 +/- ##
===========================================
+ Coverage 90.67% 90.86% +0.19%
===========================================
Files 45 45
Lines 6335 6469 +134
===========================================
+ Hits 5744 5878 +134
Misses 591 591
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
bocklund
changed the title
Z value correction
FIX: Canonicalize coordination number sorting in DAT reader
May 18, 2022
bocklund
reviewed
May 18, 2022
Yup
…On Wed, May 18, 2022 at 6:40 PM Brandon Bocklund ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pycalphad/tests/test_energy.py
<#416 (comment)>:
> +
+ F = v.Species('F-1.0',constituents={'F':1.0}, charge=-1)
+ K = v.Species('K+1.0',constituents={'K':1.0}, charge=1)
+ NI = v.Species('NI+2.0',constituents={'NI':1.0}, charge=2)
+ mod = ModelMQMQA(dbf, ["K", "NI", "F"], "LIQUID2")
+
+ assert K in mod.cations
+ assert NI in mod.cations
+ assert F in mod.anions
+
+ subs_dict={mod._X_ijkl(NI,NI,F,F):0.36820754040431064,
+ mod._X_ijkl(K,NI,F,F):0.52716983838275622,
+ mod._X_ijkl(K,K,F,F):0.10462262121293306,
+ v.T: 1600}
+
+ check_energy(mod, subs_dict, -3.35720E+05, mode="sympy") # Thermochimica energy
@jorgepazsoldanpalma <https://github.com/jorgepazsoldanpalma> just to
confirm, this energy is from Thermochimica, right?
—
Reply to this email directly, view it on GitHub
<#416 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFPINJNKJTYMAMSN42DFQZDVKVWVBANCNFSM5WJ2OEAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
bocklund
approved these changes
May 18, 2022
Thanks for the test and the fix, @jorgepazsoldanpalma! And congratulations on your first PR to PyCalphad! |
My pleasure!
…On Thu, May 19, 2022 at 12:36 PM Brandon Bocklund ***@***.***> wrote:
Thanks for the test and the fix, @jorgepazsoldanpalma
<https://github.com/jorgepazsoldanpalma>! And congratulations on your
first PR to PyCalphad!
—
Reply to this email directly, view it on GitHub
<#416 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFPINJNDD7CQLOCSZB3CPU3VKZU2JANCNFSM5WJ2OEAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
yup
…On Wed, May 18, 2022 at 6:40 PM Brandon Bocklund ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pycalphad/tests/test_energy.py
<#416 (comment)>:
> +
+ F = v.Species('F-1.0',constituents={'F':1.0}, charge=-1)
+ K = v.Species('K+1.0',constituents={'K':1.0}, charge=1)
+ NI = v.Species('NI+2.0',constituents={'NI':1.0}, charge=2)
+ mod = ModelMQMQA(dbf, ["K", "NI", "F"], "LIQUID2")
+
+ assert K in mod.cations
+ assert NI in mod.cations
+ assert F in mod.anions
+
+ subs_dict={mod._X_ijkl(NI,NI,F,F):0.36820754040431064,
+ mod._X_ijkl(K,NI,F,F):0.52716983838275622,
+ mod._X_ijkl(K,K,F,F):0.10462262121293306,
+ v.T: 1600}
+
+ check_energy(mod, subs_dict, -3.35720E+05, mode="sympy") # Thermochimica energy
@jorgepazsoldanpalma <https://github.com/jorgepazsoldanpalma> just to
confirm, this energy is from Thermochimica, right?
—
Reply to this email directly, view it on GitHub
<#416 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFPINJNKJTYMAMSN42DFQZDVKVWVBANCNFSM5WJ2OEAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This summary added from the comments in the PR)
Previously PyCalphad would alphabetically express the quadruplets and because of this if the coordination numbers of a binary quadruplet in the database were not expressed in alphabetic order in the database parameters, then the coordination numbers would be incorrectly ordered. Sometimes this would trigger a calculation of coordination number, instead of a lookup. A fix has been added to the cs_dat parser to canonically sort coordination numbers when DAT files are read.
The test makes sure that the order in which the species are entered in the DAT file does not affect the calculated energy. In this specific test the Ni is entered before the K species.