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

ENH: Enable option B and option F support for TDBs #412

Merged
merged 7 commits into from
May 11, 2022

Conversation

bocklund
Copy link
Collaborator

@bocklund bocklund commented Apr 27, 2022

Move over generate_symmetric_group from PhasesResearchLab/ESPEI#229 with tests.

Add Al-Fe database from Sundman et al. 2009 that uses option B and option F. The option B phase also has a non-option B alternative, so we can compare energies.

This PR closes #202.

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #412 (e0f2013) into develop (f8e3951) will increase coverage by 0.16%.
The diff coverage is 99.13%.

@@             Coverage Diff             @@
##           develop     #412      +/-   ##
===========================================
+ Coverage    90.67%   90.83%   +0.16%     
===========================================
  Files           45       45              
  Lines         6335     6448     +113     
===========================================
+ Hits          5744     5857     +113     
  Misses         591      591              
Impacted Files Coverage Δ
pycalphad/io/tdb.py 89.70% <96.15%> (+0.35%) ⬆️
pycalphad/core/utils.py 85.10% <100.00%> (+2.28%) ⬆️
pycalphad/io/database.py 87.24% <100.00%> (+0.44%) ⬆️
pycalphad/tests/test_calculate.py 100.00% <100.00%> (ø)
pycalphad/tests/test_database.py 100.00% <100.00%> (ø)
pycalphad/tests/test_utils.py 100.00% <100.00%> (ø)
pycalphad/codegen/sympydiff_utils.py 98.21% <0.00%> (-1.79%) ⬇️
pycalphad/model.py 91.84% <0.00%> (+0.16%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@bocklund
Copy link
Collaborator Author

This implements the symmetry options at the level of the database parameters. Maybe one thing to discuss is whether it makes more sense to do this operation at the level of a database or to only do it inside the model. Some considerations:

  • with the current approach, pycalphad-xml also should have support added for generating parameters since presumably we'll have these phase options implemented there as well (we should IMO).
  • Most model hints we treat inside models. Symmetry phase options are a special case because they correspond virtually to new parameters. However, we do have some precedent for handling the virtual symmetric parameters for ternary interactions inside the model at model-build-time.

@richardotis
Copy link
Collaborator

I was prepared to argue against implementing the parameter augmentation at the Database level because I had concerns about round-tripping. However, I see that the approach here (marking the parameters with special metadata) makes it easy to preserve the round-trip property without intruding on Model definition. Also, it keeps Model objects from needing to know about symmetry.

One concern I have is with performance. Does a multicomponent 4SL BCC or FCC phase with the symmetry options blow up in time or memory? My intuition is 'no' but I'd be interested to know how it behaves with Bengt's MPEA database, for example.

@richardotis
Copy link
Collaborator

Another question: Option B/F apply to all of a phase's model parameters (how it's implemented here), not just the energy, correct?

@bocklund
Copy link
Collaborator Author

One concern I have is with performance. Does a multicomponent 4SL BCC or FCC phase with the symmetry options blow up in time or memory? My intuition is 'no' but I'd be interested to know how it behaves with Bengt's MPEA database, for example.

Looking at databases I have access to, the biggest with symmetry options are mpea-02b.tdb (which has the most symmetry parameters) and iron-04.tdb (which has the most constituents in phases with symmetry options).

On average over 20 runs:

Database # symmetry ordering parameters Time to load database Time to add ordering parameters % time adding parameters
mpea-02b.tdb 63 1.64s 0.17s 11%
iron-04.tdb 24 3.59s 0.10s 3%

The computational complexity of generate_symmetric_group only depends on the number of sublattices that are participating in symmetry operations and the computational time is independent of the number of constituents. Time and memory should not be an issue.

The way add_phase_symmetry_ordering_parameters is implemented, we waste some time by recomputing essentially the same group. Since generate_symmetric_group operates only on the sublattices, what is inside the sublattices doesn't matter except for the set operations to determine the uniquely generated arrays. That is, constituent arrays of (("AL",), ("NI",), ("NI",), ("NI",)), (("AL", "NI"), ("*",), ("*",), ("*",)), (("FE",), ("TI",), ("TI",), ("TI",)), ("A", "B", "B", "B"), (0, 1, 1, 1), ... all will generate essentially the same symmetry group (for FCC: 4 different, symmetrically equivalent permutations). Theoretically, we could go through and find all the abstract unique symmetry patterns, call generate_symmetric_group once for each pattern, then apply that pattern to construct concrete constituent arrays for each parameter. It's unclear whether there would be a meaningful benefit

Another question: Option B/F apply to all of a phase's model parameters (how it's implemented here), not just the energy, correct?

Yes, I believe so. The Al-Fe database added to the tests has only the symmetrically distinct BMAGN parameter given for the option B phase, but has all parameters listed in the version of the 4SL bcc phase without option B. That is consistent with how partitioned parameters work.

@bocklund
Copy link
Collaborator Author

With the latest changes, you can now delete a model hint and it will effectively convert the database to not use the symmetry option.

from pycalphad import Database, variables as v
dbf = Database("mpea-02b.tdb")
del dbf.phases["FCC_4SL"].model_hints["symmetry_FCC_4SL"]
dbf.to_file("mpea-02b-noF.tdb")

However, when testing this, I noticed that calling remove_phase_symmetry_ordering_parameters has a side effect of modifying the database, so write_tdb is impure. Calling the write_tdb function will remove the symmetry parameters, which is a bug. To fix, how about either:

  1. write_tdb makes a copy of the database
  2. write_tdb doesn't remove parameters, but does a check internally to skip writing a parameter, something like if phase_obj.model_hints.get(symmetry_hint, False) and param.get("_generated_by_symmetry_option", False)

I'm leaning towards (2), but not sure. Is remove_phase_symmetry_ordering_parameters still useful to keep around if it's not used?

@richardotis
Copy link
Collaborator

I think (2) is a good approach. remove_phase_symmetry_ordering_parameters can stick around if you add a test to make sure it doesn't break over time, or you can just remove it.

@bocklund bocklund requested review from richardotis and removed request for richardotis May 11, 2022 16:30
pycalphad/tests/test_utils.py Outdated Show resolved Hide resolved
pycalphad/tests/test_calculate.py Show resolved Hide resolved
pycalphad/io/tdb.py Show resolved Hide resolved
pycalphad/core/utils.py Show resolved Hide resolved
pycalphad/tests/test_utils.py Outdated Show resolved Hide resolved
@bocklund bocklund merged commit b1a3393 into pycalphad:develop May 11, 2022
@bocklund bocklund deleted the ENH-option-BF-support branch May 11, 2022 21:24
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.

TDB symmetry options
2 participants