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: Model reference states #205

Merged
merged 29 commits into from May 10, 2019

Conversation

Projects
None yet
2 participants
@bocklund
Copy link
Collaborator

commented Mar 7, 2019

  • Adds a ReferenceState class which allows the definition of a reference phase for each pure element in the system and any fixed state variables, such as temperature or pressure.

  • Model now has a shift_reference_state method which can take these reference states and create new properties on the Model object to shift the AST for XYZ contributions XYZR (defaults to GM, HM, SM, CPM)

  • XYZ_MIX properties have been fixed in the Model class to remove all endmember contributions (closes #55). For order-disorder models, this cannot be done properly in a way that sets the reference endmember energies to zero and preserves the disordered state mixing terms, so the _MIX energies have been set to nan in that case.

  • test_energy.py has been slightly refactored to be able to test arbitrary model properties.

@bocklund

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2019

The main ideas have all been implemented and could be reviewed.
There are several outstanding items that prevent this from being merged:

  • A documentation example showing usage of the different ReferenceState objects in calculate and equilibrium,
  • Potential issue where if a user changes the AST of a model after instantiation (e.g. mod.models['idmix'] = 0) that the GMR will not update automatically. We need to set the GMR properties dynamically, e.g. setattr(self, 'GMR', property(lambda self: self.GMR - ref.GMR)), but that doesn't work because the property needs to be bound to the class. I'm pretty sure there's a way to do this, but I need to look into it.

[1] I want to ask for input for two things on this.

  1. My main blocker here is that I would like to be able to specify ReferenceStates either as a list of [ReferenceState(...), ReferenceState(...)]... that apply to all phases in the calculation or as a dict of {'FCC_A1': [ReferenceState(...), ReferenceState(...), ...}, basically following the format of unpack_kwargs. My hangup is that I'm not sure where to apply this, since the Models in calculate/equilibrium are just default dicts of either a Model subclass or a Model instance until they are pulled out and constituted in build_callables. I hesitate to propagate Model instances down to build_callables because that function shouldn't have to know anything about the internal representation of the phase or property it's building.
  2. My other question is whether the calculate/equilibrium should create copies of Model instances before applying the reference state, so that the user-passed Model objects are not modified, otherwise
# raises attribute error b/c GMR does not exist
equilibrium(dbf, comps, phases, conds, model=model_dict, output='GMR)
equilibrium(dbf, comps, phases, conds, model=model_dict, refstate=reference_states, output='GMR)

# produces the same output as above because the objects in model_dict have been changed
equilibrium(dbf, comps, phases, conds, model=model_dict, output='GMR)

If users explicitly want this behavior, they should change the reference states manually:

for phase_name in model_dict.keys():
    model_dict[phase_name].shift_reference_state(reference_states, dbf)
equilibrium(dbf, comps, phases, conds, model=model_dict, output='GMR)

# produces the same output as above
equilibrium(dbf, comps, phases, conds, model=model_dict, refstate=reference_states, output='GMR)
@richardotis

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2019

This API is great.

  1. I think all phases in a given calculation should accept a common list of ReferenceStates. The meaning of the R properties is unclear in a mixed reference state environment. My thought is that Model.reference_model shouldn't affect Model.GM at all, so the solver should be independent of a reference state shift. When the user specifies GMR or whatever R property, we should leverage the property calculation machinery to compute the reference state shift after the fact and subtract it from the SER property value. I also think if the user specifies the refstate kwarg, we should assume they want GMR implicitly (and not return GM to avoid footguns).
  2. The refstate kwarg should not mutate anything in the models kwarg.

Also, I know this isn't finished, but I notice that there is no test for fixed_statevars in ReferenceState yet.

@bocklund

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 15, 2019

Since Models are lazily instantiated via unpack_kwarg, it's tricky to know where they will be instantiated and we want to avoid mutating them. The latest they could possibly be instantiated is inbuild_callables, but I don't think the logic for copying Model instances and then calling shift_reference_state belongs in that function.

Can we defer calculate/equilibrium API to a later issue and merge this when the tests are passing?

@richardotis

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

Okay.

@bocklund bocklund added this to the 0.8 milestone Apr 22, 2019

@bocklund bocklund force-pushed the ENH-Model-reference-states branch from f53591b to b4d248d Apr 24, 2019

@bocklund bocklund requested a review from richardotis Apr 24, 2019

@richardotis
Copy link
Collaborator

left a comment

This is an impressive new feature, and it is appreciated that you've tested several key cases.

While there is no nice user-facing calculation API today, what do you think about adding an example to the documentation for computing mixing energies for some binary system? I think calling shift_reference_state in a loop on pre-computed Models, then passing those to calculate() with output='GMR' would work.

Show resolved Hide resolved pycalphad/model.py Outdated
Show resolved Hide resolved pycalphad/model.py Outdated
Show resolved Hide resolved pycalphad/tests/test_energy.py
Show resolved Hide resolved pycalphad/model.py
@bocklund

This comment has been minimized.

Copy link
Collaborator Author

commented May 1, 2019

I added an example notebook with three examples, including explaining the difference between R and _MIX properties. Added a simple test for contrib_mods.

I think this is ready for another look.

@richardotis

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

Please merge when all the tests except Windows py37 (which is deferred for this issue) pass.

bocklund added some commits May 3, 2019

bocklund added some commits May 7, 2019

@bocklund bocklund merged commit 41a473d into develop May 10, 2019

3 of 5 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 88.763%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.