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: Max phases by Gibbs phase rule accommodated #184

Merged
merged 6 commits into from Aug 17, 2018

Conversation

Projects
None yet
2 participants
@bocklund
Collaborator

bocklund commented Aug 17, 2018

  • Mostly straightforward fix that closes #170 by padding the vertex to be of size C+2 instead of C (for C components).
  • Includes a test of a eutectic point in Pb-Sn.
  • Modestly improved hyperplane documentation

This requires a hack in hyperplane because hyperplane constructs a C-simplex hyperplane corresponding to the chemical potentials, however the phase fraction and point index arrays are now padded to accommodate C+2 phases instead of C phases. So I set the point index to zero (which pulls in the phase, site fracs, etc. associated with that point) and the phase fraction of those points to zero. I'm not sure if this was correct or what implications there are for doing this, but the tests pass...?

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Aug 17, 2018

I'm not expecting the CI tests to pass until this is rebased on #182 that updates SymPy requirements

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Aug 17, 2018

Does this still work if you set the maximum number of phases to c+1 instead of c+2? I support this change, but I want to note that the current implementation already respects the Gibbs phase rule, which is c + 2 - 2 = c phases can coexist at once. We lose two degrees of freedom from fixing T and P.

The reason we want to artificially increase the maximum phases allowed is because we want to accommodate the degenerate degree of freedom at the invariant reactions. (The solution this change gives will be "correct," but the phase fractions won't be meaningful since multiple values would give the same Gibbs energy on the invariant line.)

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Aug 17, 2018

I changed C+2 to C+1 and tests pass locally.

@richardotis

Minor comments only. Merge when ready

-----
M: number of energy points that have been sampled
N: number of components
P: N+2, max phases by gibbs phase rule

This comment has been minimized.

@richardotis

richardotis Aug 17, 2018

Collaborator

Needs to be changed to N+1. Maybe choose another letter besides P to represent this quantity, so it isn't easy to confuse with a thermodynamic variable

This comment has been minimized.

@richardotis

richardotis Aug 17, 2018

Collaborator

(though we do already use N, so...)

This comment has been minimized.

@bocklund

bocklund Aug 17, 2018

Collaborator

I think defining them prevents confusion for overloading (but not having any labels was confusing).

result_simplex[:num_components] = best_guess_simplex
# Hack to enforce Gibbs phase rule, shape of result is comp+2, shape of hyperplane is comp

This comment has been minimized.

@richardotis

richardotis Aug 17, 2018

Collaborator

comp+1 now

@bocklund bocklund force-pushed the fix-gibbs-phase-rule branch from 49b11a2 to c4ccb85 Aug 17, 2018

@bocklund bocklund merged commit 916ad73 into develop Aug 17, 2018

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.4%) to 87.757%
Details

@bocklund bocklund deleted the fix-gibbs-phase-rule branch Aug 17, 2018

@bocklund bocklund added this to the 0.7.1 milestone Aug 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment