FIX: MQMQA: Handle species that have mole numbers that are higher than 1

[jorgepazsoldanpalma]

In the following commit, changes have been made so that the MQMQA model can handle species that have mole numbers that are higher than 1 (ie. dimers,trimers). In order to handle this correctly the _n_i function, ideal mixing function and some energy tests had to be adjusted. Additionally, two new tests were introduced which make sure the species with multiple moles and vacancies in the MQMQA model are correctly described.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (071c50a) 90.46% compared to head (505a758) 90.61%.

❗ Current head 505a758 differs from pull request most recent head b292b18. Consider uploading reports for the commit b292b18 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #496      +/-   ##
===========================================
+ Coverage    90.46%   90.61%   +0.14%     
===========================================
  Files           50       50              
  Lines         7868     7894      +26     
===========================================
+ Hits          7118     7153      +35     
+ Misses         750      741       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bocklund]

@jorgepazsoldanpalma is the change from Fe2+ -> Fe+2, Fe3+ -> Fe+3, Sb3+ -> Sb+3 in the Shishin_Fe-Sb-O-S_slag.dat database significant?

I tried to change just Sb3+ back to Sb+3 and the tests broke. If I'm understanding correctly, the changes in this pull request change the semantics about what is a valid name for a species (or the semantics of how a species string name is turned into a Species object). My understanding is that current practice in the develop branch is that we treat Fe2+ in the DAT as a string name of the species and we get the charge state from the following lines.

The key question I have is: do FactSage and Thermochemica work with both versions (both Sb3+ and Sb+3)? I.e., does this PR introduce a regression in the databases we can handle?

[jorgepazsoldanpalma]

Hi @bocklund ! The changes I made to the species were due to the fact that when the .dat file reads the species as Fe2+ it believes that the species should have a constituent of 2 rather than 1. That was the main reason I changed it. As you correctly pointed out, the lines below the specie labeling do in fact give information regarding the respective charge of the species.

[bocklund]

Hi @bocklund ! The changes I made to the species were due to the fact that when the .dat file reads the species as Fe2+ it believes that the species should have a constituent of 2 rather than 1. That was the main reason I changed it. As you correctly pointed out, the lines below the specie labeling do in fact give information regarding the respective charge of the species.

Is Fe2+ valid in Thermochimica and FactSage?

The test gives the Thermochimica value, which implies that Fe2+ was working and giving the matching answer in PyCalphad. Is the answer still the same in the test when using the unchanged DAT?

[jorgepazsoldanpalma]

@bocklund The originaly reason I changed the lines, if I remember correctly, was because the energies were not matching due to the fact that the parser was interpreting the Fe2+ as being a species with charge 2 and a constituent of 2 rather than 1. Perhaps this bug in the parser has been addressed?

[bocklund]

@jorgepazsoldanpalma You added some new tests for databases containing Be2 or Al2, which is good. The fact that you needed to change this database to be Fe+2 when Thermochimica/FactSage can use the Fe2+ version means that something about the implementation of this feature for mole numbers ≠ 1 is wrong.

Both the new Be2/Al2 tests that were added should pass and the existing tests that use the Fe2+-style should work (and should agree with the output of using Fe2+-style database in Thermochimica/FactSage)

[bocklund]

If you change the database base to Fe2+ then PyCalphad raises and exception in the tests that use this database, but it should still compute the same values that the other software give.

[jorgepazsoldanpalma]

@bocklund, just to make sure I am understanding your comments. You are saying that when you change the species from Fe+2 to Fe2+ PyCalphad just raises and exception but still correctly calculates the energies. If that is what you are saying then that is not the case at all for me. I obtain an error in which the energies are not the same with the values from Thermochimica. Now this is my understanding the of the error so far on my end and in the branch I am proposing.
when I made the first implementation to in the MQMQA model so that it can handle species whose mass was not equal to 1, I noticed that I kept failing with the Shinshin tests. Taking a closer look the error was consistently a mismatch between the energies that were in Thermochimica and PyCalphad. Below is an example of how the the species were expressed originally

@select_database("Shishin_Fe-Sb-O-S_slag.dat")
def test_MQMQA_SUBQ_Q_mixing_Sb_O_S_1000K(load_database):
dbf = load_database()

FE2 = v.Species("FE2++2.0", constituents={"FE": 2.0}, charge=2)
FE3 = v.Species("FE3++3.0", constituents={"FE": 3.0}, charge=3)
SB3 = v.Species("SB3++3.0", constituents={"SB": 3.0}, charge=3)
O = v.Species("O-2.0", constituents={"O": 1.0}, charge=-2)
S = v.Species("S-2.0", constituents={"S": 1.0}, charge=-2)
mod = ModelMQMQA(dbf, ["SB", "S", "O"], "SLAG-LIQ")

This is wrong since what this is implying is the Fe2 is in fact a dimer (2 atoms/moles of Fe) of charge 2 rather than 1 mole of Fe that has a charge of 2. Now the reason this test was not raising issues in the past was in fact the error addressed in this branch. The code assummed all species had a constituent of 1 no matter was assigned to the species itself. This is why in the LiF-BeF2 system is was incorrectly normalizing the Gibbs energy of the quadruplets with Be dimers in it. In this current implemetation I make the code read the species constituent so that it can correctly differentiate dimers and monomers, and hence the test from Shishin fails now. The reason why I changed the configuration of the species is because I know the code interprets a number next to a species to be the constituent number and instead of fixing the fundamental problem (which seems to be how the .dat file address how mass is attributed to species ) I was lazy and quickly changed the expressions of the species in the database. I hope that now my actions are more clear.

[bocklund]

The reason why I changed the configuration of the species is because I know the code interprets a number next to a species to be the constituent number and instead of fixing the fundamental problem (which seems to be how the .dat file address how mass is attributed to species ) I was lazy and quickly changed the expressions of the species in the database. I hope that now my actions are more clear.

Yeah, I think this is what I'm getting at.

To merge this pull request we need to ensure that we:

1. Do not break existing correct behavior (i.e. having Shishin species of Fe2+, Fe3+, and Sb3+ needs to work and be in agreement with Thermochemica).
2. Add support for the dimer species like Be2

Right now we are violating (1) at the expense of (2), but I we should merge the implementation that satisfies both (1) and (2). Let me know if I'm understanding correctly and whether that makes sense.

If it's helpful at all, I did the work to revert the Shishin database and tests and pushed the changes up to your branch here. We can successfully merge this pull request when the implementation (of the DAT/XML parsers and the ModelMQMQA class) makes all the tests (new and old) pass without modifying the databases.

[jorgepazsoldanpalma]

Roger. I'll get to that implementation asap! Thanks!