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: Make minimizer state more internally consistent #410

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

bocklund
Copy link
Collaborator

This PR moves advance_state to be the last step in the minimization loop.

The main reason to make this change is that we want the chemical potentials that we get from solve_state to be consistent with the current site fractions, phase amounts, and state variables when we check for convergence, change phases, and return a solution. Right now, we apply the corrections to site fractions, phase amounts, and state variables after solving for the chemical potentials with the current values, so they are effectively one step ahead of the chemical potentials.

This fixes #372 as the driving forces should now always be zero in change_phases, as the chemical potentials are now consistent with the current site fractions.

Summary of changes:

  1. If the phases change, we don't advance the state. It probably makes sense to be doing this so that we only change one thing at a time (i.e. we don't want to change the site fractions/phase amounts/state variables and the phases).
  2. We set the numerical limit to phase amounts in the adaptive step size determination in advance_state to something non-zero to avoid divide by zero cases.
  3. Slightly adjust the convergence tolerance. Without this adjustment, the rose test was failing. Because we were returning site fractions for the step ahead, I think the root cause was that the next step would converge to the correct energy in the test, so slightly tightening the tolerances seemed to help get there. Still, the limiting factor for something like ALLOWED_DELTA_Y = 1e-09 seems to be the charged species test.

Needed a slight adjustment in the convergence tolerances as the feasibility check is checking how things changed as a result of the last iteration
@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #410 (32c4dda) into develop (6bcc902) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #410   +/-   ##
========================================
  Coverage    90.67%   90.67%           
========================================
  Files           45       45           
  Lines         6335     6335           
========================================
  Hits          5744     5744           
  Misses         591      591           

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

@bocklund bocklund merged commit f8e3951 into pycalphad:develop Apr 19, 2022
@bocklund bocklund deleted the minimizer-state-consistency branch April 19, 2022 01:28
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.

pycalphad.core.minimizer.change_phases can give IndexError: list index out of range
2 participants