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

FIX: eqcalculate: Calculate degrees of freedom based on Model #270

Merged
merged 2 commits into from Apr 10, 2020

Conversation

richardotis
Copy link
Collaborator

When the Phase is defined in a Database with many more components than are defined
to be active in the present calculation, _eqcalculate() would incorrectly calculate degrees of freedom based on all possible components, instead of the active ones.

This would then lead to errors inside calculate(), when the length of the passed points array would exceed the maximum_internal_dof.

This fix does two things:

  1. Calculates degrees of freedom using the already-passed models object, which is based on the active components.
  2. Fixes unpack_phases to return only the active phases, instead of returning nothing and causing a fallback to all phases in the Database. (This wasn't a correctness issue, but it did cause unnecessary computation.)

…tabase.

When the Phase is defined in a Database with many more components than are defined
to be active in the present calculation, _eqcalculate() would incorrectly calculate degrees of freedom based on all possible components, instead of the active ones.
pycalphad/core/equilibrium.py Outdated Show resolved Hide resolved
@richardotis richardotis requested a review from bocklund Apr 10, 2020
Copy link
Collaborator

@bocklund bocklund left a comment

Choose a reason for hiding this comment

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

Good that you added the Model argument. I think the only reason this was working for non-standard Models before was that the callables were already being built by equilibrium and passed down to _eqcalculate.

Removing the dead mode code is optional (though we should probably remove that from the calculate API for the next breaking change)

pycalphad/core/equilibrium.py Show resolved Hide resolved
@richardotis richardotis merged commit 5c239f1 into develop Apr 10, 2020
bocklund added a commit that referenced this pull request Apr 11, 2020
In reviewing #270, I overlooked the fact that `calculate` takes the argument `model`, which a caller could pass as a keyword argument to `_eqcalculate`. #270 changed `_eqcalculate` to explicitly take a `models` argument and pass it to `calculate`.

This preserves the behavior from #270 and stays backwards compatible if anyone was using `model` as a keyword argument that got passed through to `calculate`.
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.

None yet

2 participants