Navigation Menu

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

Solver improvements via IPOPT #124

Merged
merged 101 commits into from Nov 25, 2017
Merged

Solver improvements via IPOPT #124

merged 101 commits into from Nov 25, 2017

Conversation

richardotis
Copy link
Collaborator

@richardotis richardotis commented Sep 14, 2017

This really big PR does a bunch of things in the core:

  • Adds the NLP solver IPOPT and its Cython wrapper cyipopt as dependencies (Conda-forge support for ipopt on Windows is pending)
  • Removes the majority of the old eqsolver core, while leaving the simplistic treatment of global minimization intact (for now)
  • Adds deepcopy support for CompositionSet
  • Creates new Problem and Solver abstractions, which will make it easier to extend the solver in the future to handle different types of problems and constraints
  • Tweaks minimum value constants to improve convergence
  • Make some equilibrium tests more restrictive

This PR has not been checked against any of the phase diagram notebooks yet.

richardotis and others added 30 commits May 19, 2017 18:27
…memory

Conflicts:
	pycalphad/core/eqsolver.pyx
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.

Great work.

Once these are fixed, we are just waiting on CI for this to be merged, yes?

@@ -1,8 +1,13 @@
# cython: linetrace=True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should remove # cython: linetrace=True and # cython: profile=True for performance.

saved_trial = argmax(smallest_fractions)
#print('saved_trial', saved_trial)
if smallest_fractions[saved_trial] < -num_components:
print('hyperplane: No further progress is possible')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented out print statements (lines 127-129) and also the 'hyperplane: No further progress is possible' on line 134

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition lines 142, 143, 145, 147, 160.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to uncomment and print if verbose=True for any of these that you would like.

for i in range(num_components):
trial_simplices[i, i] = min_df
if lowest_df[0] > -1e-8:
break
if lowest_df[0] < -1e-8:
raise ValueError('Max hull iterations exceeded. Remaining driving force: ', lowest_df[0])
#if lowest_df[0] < -1e-8:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be commented? Remove if necessary.

constraint_offset += 1
return np.array(constraint_jac)

#def hessian(self, x, lagrange, obj_factor):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need lines 192-214?

@@ -46,13 +46,13 @@ cdef public class CompiledModel(object)[type CompiledModelType, object CompiledM
double *eval_row, double[:] parameters) nogil
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think cymem is dead now so the following needs to be cleaned up

compiled_model.pxd

  • cdef public Pool mem in line 39.
  • As well as the import in line 2.

In compiled_model.pyx:

  • self.mem = Pool()(line 56)
  • import in line 2
  • inst.mem in line 1071
  • Pool() in line 1018

In addition the

  • cymem.pyx and pxd
  • cymem license
  • cymem cythonize in setup.py

length_scale = max(length_scale, 1e-9)
# nlp.addOption(b'derivative_test', b'first-order')
# nlp.addOption(b'check_derivatives_for_naninf', b'yes')
MAX_SOLVE_DRIVING_FORCE = 1e-4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the MAX_SOLVE_DRIVING_FORCE and other tolerances here be set in the constants module? That way it could be reasonably found and changed at compile time. I think you mentioned moving towards being able to set these at run time, which would be more easily supported by that kind of change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could see advanced users (and myself) wanting any easy way to tweak the solver performance and recompile.

converged = False
elif info['status'] < 0:
if self.verbose:
print('Calculation Failed: ', cur_conds, info['status_msg'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should calculation failures be noisier as in the current solver? Or is this a different kind of error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This error means that ipopt failed to find a solution, which should be pretty rare now.

assert np.all(np.abs(mass_error) < 0.01)
assert_allclose(eq.GM.values, -105871.20, atol=0.1)
assert_allclose(eq.MU.values.flatten(), [-104655.532294, -142591.644379, -82905.085459], atol=0.1)
#assert_allclose(eq.GM.values, -105871.54, atol=1.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove the commented out assertion?

@@ -217,6 +221,9 @@ def test_eq_issue43_chempots_misc_gap():
{v.X('AL'): .1246, v.X('CR'): 1e-9, v.T: 1273, v.P: 101325},
verbose=True)
chempots = 8.31451 * np.squeeze(eq['T'].values) * np.array([[[[[-19.47631644, -25.71249032, -6.0706158]]]]])
mass_error = np.nansum(np.squeeze(eq.NP * eq.X), axis=-2) - \
[0.1246, 1e-9, 1-(.1246+1e-9)]
print('Mass error', mass_error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be doing some assertion here with the mass error or can it be removed?

setup.py Outdated
@@ -35,7 +36,7 @@ def read(fname):
long_description=read('README.rst'),
url='https://pycalphad.org/',
install_requires=['matplotlib', 'pandas', 'xarray!=0.8', 'sympy', 'pyparsing', 'Cython>=0.24',
'tinydb', 'scipy', 'numpy>=1.9', 'dask[complete]>=0.10', 'dill'],
'tinydb', 'scipy', 'numpy>=1.9', 'dask[complete]>=0.10', 'dill', 'ipopt'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the PyPI ipopt is a little behind cyipopt. Do we need to be involved in maintaining ipopt bindings PyPI as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The friendly @moorepants is currently maintaining the bindings on PyPI, so I think if we sell upstream on Windows support, the release will follow shortly.

Choose a reason for hiding this comment

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

The previous developer has given me access to the ipopt pypi but i've never updated it, just been using conda-forge's version which is a fork. I think the right move is to start using the name cyipopt on pypi and push from the fork: https://github.com/matthias-k/cyipopt, and abandone the ipopt name on pypi.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@moorepants I support you on the name change. Moving forward with it now would be great since we could write it in before pycalphad's next major release (T-minus ~1 week).

Choose a reason for hiding this comment

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

Someone needs to make a PR with the name changes to: https://github.com/matthias-k/cyipopt. There is already an issue open about it. We agreed that the distribution name and the package name (the import call) should be cyipopt. I am unlikely to have time in a week to do that and I don't have merge rights on the repo.

@bocklund
Copy link
Collaborator

Just waiting on cyipopt for Windows. Otherwise I think this is ready to merge.

@richardotis richardotis merged commit 40973ee into develop Nov 25, 2017
richardotis added a commit that referenced this pull request Nov 26, 2017
This really big commit does a bunch of things in the core:
- Adds the NLP solver IPOPT and its Cython wrapper cyipopt as dependencies
- Removes the majority of the old eqsolver core, while leaving the simplistic treatment of global minimization intact (for now)
- Adds deepcopy support for CompositionSet
- Creates new `Problem` and `Solver` abstractions, which will make it easier to extend the solver in the future to handle different types of problems and constraints
- Tweaks minimum value constants to improve convergence
- Make some equilibrium tests more restrictive

* WIP: Initial memory cleanup work

* WIP: Profiling and memory and speed perf attempts

* WIP: eqsolver constraints and add_new_phases: perf improvements by profiling

* WIP: eqsolver: Remove debugging code

* WIP: nogil in eval_energy

* WIP: eval_energy nogil

* WIP: _eval_disordered_energy and _eval_energy_gradient nogil

* WIP: revert _eval_energy_gradient nogil

* WIP: _eval_energy_gradient nogil fixed with calloc

* WIP: eval_energy_gradient nogil

* WIP: eval_energy_hessian and update nogil

* WIP: eqsolver: type declaration cleanup

* ENH: eqsolver: Add more line search iterations for performance

* WIP: eval_energy: change to pointer for out argument

* WIP: eval_energy_gradient and eval_energy fully release gil

* WIP: eval_energy_hessian fully release gil

* FIX: eqsolver: Add guard for singular matrices during solve

* FIX: eqsolver: Convergence at lower point density by fixing bugs in add-remove-phases step of computation

* FIX: eqsolver: Performance and stability improvements


* WIP: Snapshot with all but one test passing

* REF: Create Solver class and abstract away some of the details from the main equilibrium solve loop

* ENH: CompositionSet: Add GM to CompositionSet.__str__

* FIX: hyperplane: Memory corruption during phase fraction calculation

* FIX: eqsolver: Inf norm calculation

* FIX/WIP: solver: Tweak complementarity tol to get convergence on one test, but break test_rose_nine.

* TST/FIX: Add cyipopt to dependencies

* FIX: Remove NumPyPrinter due to sympy 1.1 incompatibility. Other sympy 1.1 import changes. Fixes gh-105.

* MAINT: eqsolver: Remove dead code

* FIX: add_new_phases: Misalignment of phase fractions array due to non-deterministic sorting of simplex vertex index array

* WIP: add_new_phases: Force endmembers to always be candidates for inclusion

* WIP: Continued attempts at global minimization improvement.

* WIP: add_new_phases: Add all stable phases plus the phase with the largest driving force.

* ENH: CompositionSet: deepcopy support

* WIP: eqsolver: Clean up and simplify global minimization code. Doesn't yet work completely but many tests pass and the performance hit is improved.

* FIX/WIP: Get tests passing at pdens=500 with ipopt. Use a two-step approach to improve ipopt convergence.

* WIP: eqsolver: For pdens=50, pass all tests with solver tweaks for cases near boundary.

* DOC/FIX: BinaryExamples: Bump results and fix error in energy plot.

* FIX: equilibrium: Reset default pdens to 500 to address ternary miscibility gap detection

* BLD/FIX: appveyor: Bump miniconda to fix broken conda

* MAINT: core: Remove obsolete cymem-related code

* MAINT: compiled_model: Remove cython debugging directives

* MAINT: hyperplane: Remove extraneous print statements

* MAINT: Problem: Remove extraneous comments on hessian implementation

* MAINT: solver: Remove extraneous commented-out print statements

* TST/MAINT: test_equilibrium: Clean up assertions

* BLD/TST: travis: Drop py34 and disable Dockerization to ease dependency handling

* MAINT: core: Refactor solver constants to be in core.constants

* FIX/TST: Use matplotlib 'Agg' backend in tests to avoid issues no DISPLAY on CI

* BLD: appveyor: Add msys2 and pycalphad channels to enable ipopt

* BLD/WIP: appveyor: Fix conda channel add

* FIX: equilibrium: dask module rename compat

* FIX: calculate: Remove fast_concat due to reliance on xarray internal API

* FIX: custom_autowrap: Import correctly on Windows

* FIX: solver: Increase ipopt max_iter to fix Windows

* BLD: appveyor: Bump py27 to 64-bit
@bocklund bocklund deleted the fix_memory branch November 30, 2017 22:48
@richardotis richardotis added this to Waiting in Roadmap Feb 26, 2018
@richardotis richardotis removed this from Waiting in Roadmap Feb 26, 2018
bocklund added a commit to bocklund/pycalphad that referenced this pull request Aug 17, 2021
…rties (pycalphad#124)

* xfail a test for dataset checking species

* Test for non-equilibrium thermochemical data

* Fix for searching for non-equilibrium thermochemical data

* Move update_phase_records

* Add WIP test for thermochemical error

* WIP: approximate equilibrium is working

* Fix phase records

* Set up weights

* Add test for equilibrium thermochemical data with species

* Fix bug for multiple potentials

* Add test for property outside of standard properties

* Raise a value error if trying to calculate a per-phase property

* Fix propagating changed parameters through

* EmceeOptimizer should now use the equilibrium thermochemical error

* Fix for case where there's no equilibrium thermochemical data

* Basic dataset checking for equilibrium thermochemical datasets

* Fix for symbols in _eqcalculate that were converted to SymPy Symbols

* Try pinning cyipopt version to a valid build

* Check that a dataset is one of equilibrium, zpf, or activity.

* Support data_weights in equilibrium thermochemical error

* Cleanup

* Bail out early for equilibrium thermochemical data if infinity or calculation error

* Revert "Try pinning cyipopt version to a valid build"

This reverts commit 497b18b70655643b4eb9896a290c5a46e9207a9d.

* Debug commit for CI

* pip install with no deps

* pin ipopt for correctness issue

* Revert "Debug commit for CI"

This reverts commit f4fd55228223075661fb590901335f03118edec7.

* Cleanup unused packages and dead comments.

* Update build_eqpropdata types and docstring

* Update get_equilibrium_thermochemical_data docstring and types

* Docstring and types calc_prop_differences

* Update docstring/types and remove error kwarg from calculate_equilibrium_thermochemical_probability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants