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

WIP: Chemical potential conditions support #200

Merged
merged 106 commits into from Mar 4, 2019

Conversation

Projects
None yet
2 participants
@richardotis
Copy link
Collaborator

commented Jan 31, 2019

This PR adds a new, flexible constraint system via JIT-compiled functions. This new flexibility is used to add support for chemical potentials as conditions in an equilibrium calculation.

richardotis added some commits Apr 16, 2018

WIP: constraints/eqsolver/equilibrium/Problem/Model: Connect JIT cons…
…traint functions to solver. Still many broken tests

richardotis added some commits Feb 2, 2019

@richardotis

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 24, 2019

Thank you for the review. The quality of the code has been significantly improved.

  1. Yes, we can do grids of chemical potential calculations now. The approach for phase diagrams is largely up to you since I see no reason to put effort into the original approach with fast mapping around the corner. One question I don't know yet is the behavior for chemical potential conditions in the very dilute case (~1e-20 and below). I have not tested that case.
  2. Yes, I agree. I would like to defer this until the next advanced conditions PR so I can confirm no more design changes will be necessary in the short term.

In addition to another round of review in light of these revisions, I'd also like your concurrence explicitly on the proposed temporary SkipTest for test_dataset_can_hold_maximum_phases_allowed_by_gibbs_phase_rule.

While making changes to the solver to improve convergence, I found that the results of this test became unreliable and platform-dependent, requiring minute changes in the calculation temperature to resolve. I now believe that this test working at all was a bug, and for the given set of conditions equilibrium should never return more than two phases. Instead, the test should be rewritten to use NP conditions with temperature as a free variable. I propose that we skip this test for now and rewrite it for the NP PR.

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

@bocklund

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

In addition to another round of review in light of these revisions, I'd also like your concurrence explicitly on the proposed temporary SkipTest for test_dataset_can_hold_maximum_phases_allowed_by_gibbs_phase_rule.

While making changes to the solver to improve convergence, I found that the results of this test became unreliable and platform-dependent, requiring minute changes in the calculation temperature to resolve. I now believe that this test working at all was a bug, and for the given set of conditions equilibrium should never return more than two phases. Instead, the test should be rewritten to use NP conditions with temperature as a free variable. I propose that we skip this test for now and rewrite it for the NP PR.

I think we have discussed this offline in the past and that freeing up another condition (T in this case) is the correct approach.

@richardotis richardotis merged commit 6b5e905 into develop Mar 4, 2019

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
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.6%) to 88.47%
Details

@richardotis richardotis referenced this pull request Apr 24, 2019

Merged

Package updates #211

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.