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: Support PhaseRecord pickling, switch SymEngine backend to LLVM (configurable) #264

Merged
merged 23 commits into from Mar 5, 2020

Conversation

@bocklund
Copy link
Collaborator

bocklund commented Feb 20, 2020

This PR includes the following changes:

  1. PhaseRecords can now be pickled, provided that all callables are built with LLVM callables. This is from adding internal references (e.g. ofunc_ to to callable functions that are pickled and reused to create the FastFunction objects when unpickling.
  2. LLVM callables are now the default everywhere, but this can be configured at runtime or through the build_functions or build_constraint_functions function APIs.
  3. Improved module documentation for sympydiff_utils and build_functions/build_constraint_functions.
  4. Updates to pin to symengine=0.6. This drastically improves the compile time performance for some of the worst-case systems and makes the compile times go from impossible on LLVM (>20,000 sec) to possible (~30-60 sec).
  5. Updates releasing docs and GH PR template to include a checklist item for updating dependencies everywhere (this has been missed in release and tests few times)

Some performance benchmarks for the test suite:

Lambda: 138 s
LLVM opt_level = 3: 174 s
LLVM opt_level=0: 135 s
LLVM opt_level=3 everywhere, Lambda Hessians: 125 s

From this data, it does appear that there's some runtime/compile time performance trade off, and the most reasonable defaults is LLVM everywhere (so everything can be pickled).

I'm pretty happy with the design and ability to override the options, but one potential gotcha is that if build_functions is called and the result cached, a subsequent change to the DEFAULT_LAMBDIFY_XYZ options will not take effect for the cached function (non-cached functions will work as expected). I think this is a pretty small tradeoff for the flexibility of this approach, and the fix for the aforementioned "gotcha" is just to restart the Python kernel and don't compile until the global DEFAULT_LAMBDIFY_XYZ is changed.

bocklund added 19 commits Feb 10, 2020
* Refactor _get_lambdify_options, with runtime changeable defaults.
* Improve sympydiff_utils module docstring
* Move build_constraint_functions to sympydiff_utils, since it compiles
…ng cyipopt"

This reverts commit 49ff4c1.
bocklund added a commit to PhasesResearchLab/ESPEI that referenced this pull request Feb 21, 2020
…iving force (#115)

Calculating the driving force error for phase boundary data is typically the most computationally intensive part of running ESPEI.

** Requires pycalphad/pycalphad#264 to run these commits in parallel!**

This PR tries to optimize (and simplify) calculating the ZPF error by doing the following:

1. Introduces a `PhaseRegion` NamedTuple that describes the data to calculate for each phase boundary set, instead of passing around all the data lists disparately. 
    * This significantly reduces the complexity of the driving force calculation. 
    * The `PhaseRegion` gets its own PhaseRecords for the calculation and contains all the relevant equilibrium arguments for that specific data. This therefore fixes a bug for mixing data types across different systems, e.g. fitting a ternary database with binary data, which used to fail because the callables/PhaseRecords would be built for the ternary, but there were only composition information for the binary and the PhaseRecords were not re-computed each time. For this reason, pycalphad/pycalphad#264 is required to run ESPEI in parallel, since SymEngine Lambda compiled objects cannot be pickled.
    * PhaseRecords are now updated for each parameter set that is calculated. This is an optimization that prevents recalculating the constraints and recompiling the constraints every time equilibrium is called, since each tieline point in the PhaseRegion will always have the same conditions and therefore constraints.
2. "Shadow functions" are implemented that shadow pycalphad's `calculate` (`calculate_`) and `equilibrium` (`equilibrium_`). These are optimized versions of pycalphad's functions that operate directly on the species, phase records, and other pycalphad internal objects as much as possible to optimize calls.
3. A `no_op_equilibrium_` "shadow function" (it doesn't really shadow anything, but exists in the same vein) which doesn't actually call `solve_eq_at_conditions` and instead just treats the `starting_point` solution as the equilibrium solution. This can give large speedups (I have already seen 3x-10x for ZPF data in certain tests systems)
4. An `approximate_equilibrium` setting in the ESPEI yaml files, which defaults to False. 

Closes #99
@bocklund bocklund added this to the 0.8.2 milestone Feb 21, 2020
@richardotis

This comment has been minimized.

Copy link
Collaborator

richardotis commented Feb 28, 2020

Does it need a test?

@richardotis

This comment has been minimized.

Copy link
Collaborator

richardotis commented Mar 5, 2020

What about PhaseRecord pickling itself? Or is that too micro of a test?

@bocklund

This comment has been minimized.

Copy link
Collaborator Author

bocklund commented Mar 5, 2020

Oh yeah, I forgot about that

@bocklund

This comment has been minimized.

Copy link
Collaborator Author

bocklund commented Mar 5, 2020

@richardotis ready to merge this with the added tests?

@richardotis

This comment has been minimized.

Copy link
Collaborator

richardotis commented Mar 5, 2020

Please merge

@bocklund bocklund merged commit 9f4aff4 into develop Mar 5, 2020
5 checks passed
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.3%) to 86.257%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.