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

MAINT: Refactor solver internals #357

Merged
merged 6 commits into from
Jun 8, 2021

Conversation

bocklund
Copy link
Collaborator

@bocklund bocklund commented Jun 8, 2021

  • Remove Problem and update the Solver.solve API to Solver.solve(composition_sets, conditions).
  • Combine _solve_and_update_if_converged and pointsolve into solve_and_update. I'm not sure solve_and_update being a cpdef function would impact performance at all compared to _solve_and_update_if_converged being a cdef function. _solve_and_update_if_converged was calling out to our pure Python Solver() class anyway.
  • Remove the need to pass around the list of active Species objects in the solver internals. Species were primarily used to determine the active, system-wide, non-vacant pure elements. Now the code uses the non-vacant pure elements in the
    PhaseRecord.nonvacant_elements attribute. This implies that PhaseRecord.components (of type: List[Species]) must be system-wide active species, i.e. mixing and matching CompositionSet/PhaseRecord objects from different active species subsets of a system is not allowed. This was already enforced implicitly via build_phase_records. Mixing and matching PhaseRecords in the same calculation would probably error or give silently incorrect results today, so this isn't a regression, but it does limit our ability to change this later without changing the API again.

I'm open to thoughts/suggestions.

  • Is solve_and_update useful enough compared to a user (and the internals) just calling Solver.solve() and update_composition_sets manually? I'd imagine that most of the time a user who is using the solve_and_update would want the composition sets to be removed automatically, but it's only one extra line of code to update and remove via update_composition_sets.

This implies a tighter, explicit coupling between the solver
and the CompositionSet/PhaseRecords objects. We make the assumption that
PhaseRecord.nonvacant_elements are the non-vacanct pure elements for the
_system_, not just the local phase it describes. This was already
implictly happening in build_phase_records and it's unclear whether
there was implicit tight coupling.

The main thing here is that this _explictly_ disallows mixing
PhaseRecord objects built with different subsets of the system marked
as "active".
@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #357 (39bf94b) into develop (b2cf3c7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #357   +/-   ##
========================================
  Coverage    89.75%   89.75%           
========================================
  Files           44       44           
  Lines         4275     4275           
========================================
  Hits          3837     3837           
  Misses         438      438           
Impacted Files Coverage Δ
pycalphad/codegen/callables.py 100.00% <ø> (ø)
pycalphad/core/equilibrium.py 94.32% <100.00%> (ø)
pycalphad/core/solver.py 100.00% <100.00%> (ø)
pycalphad/plot/binary/map.py 86.66% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2cf3c7...39bf94b. Read the comment docs.

@bocklund bocklund requested a review from richardotis June 8, 2021 13:37
@richardotis
Copy link
Collaborator

I'll review soon, but just a few preliminary responses to your main points:

  • Combine _solve_and_update_if_converged and pointsolve into solve_and_update. I'm not sure solve_and_update being a cpdef function would impact performance at all compared to _solve_and_update_if_converged being a cdef function. _solve_and_update_if_converged was calling out to our pure Python Solver() class anyway.

The performance impact should be negligible given that we are calling back into Python anyway, as you note. Even if we fully Cythonize the path from solve_and_update to find_solution, calling a cpdef function from Cython should be about the same as calling a cdef function from Cython.

  • Remove the need to pass around the list of active Species objects in the solver internals. Species were primarily used to determine the active, system-wide, non-vacant pure elements. Now the code uses the non-vacant pure elements in the
    PhaseRecord.nonvacant_elements attribute. This implies that PhaseRecord.components (of type: List[Species]) must be system-wide active species, i.e. mixing and matching CompositionSet/PhaseRecord objects from different active species subsets of a system is not allowed. This was already enforced implicitly via build_phase_records. Mixing and matching PhaseRecords in the same calculation would probably error or give silently incorrect results today, so this isn't a regression, but it does limit our ability to change this later without changing the API again.

This assumption is correct. In my "local equilibrium" test code, I added an assertion to verify that PhaseRecord.components was the same for all specified PhaseRecord objects:

# PhaseRecord components should be the same for all composition sets
all_pr_components = [cs.phase_record.components for cs in composition_sets]
assert all_pr_components.count(all_pr_components[0]) == len(all_pr_components)
  • Is solve_and_update useful enough compared to a user (and the internals) just calling Solver.solve() and update_composition_sets manually? I'd imagine that most of the time a user who is using the solve_and_update would want the composition sets to be removed automatically, but it's only one extra line of code to update and remove via update_composition_sets.

Great question. For stepping/mapping, deleting the metastable composition sets is not necessarily desirable (since they may come back). For point computations, it could be cleaner UX for metastable composition sets to not be shown. It might be that this could be addressed with a better repr for CompositionSet, SolverResult, or by wrapping composition_sets in a list-like container which has a special repr. In any case, this distinction is not presently surfaced in the API at all and should be addressed.

pycalphad/core/eqsolver.pyx Outdated Show resolved Hide resolved
pycalphad/core/eqsolver.pyx Outdated Show resolved Hide resolved
@bocklund
Copy link
Collaborator Author

bocklund commented Jun 8, 2021

Screen Shot 2021-06-08 at 4 48 03 PM

Screen Shot 2021-06-08 at 4 49 49 PM

@richardotis thoughts on which to use for the signature?

@richardotis
Copy link
Collaborator

I think the top one (with indent) is fine.

@bocklund bocklund merged commit f5bd032 into pycalphad:develop Jun 8, 2021
@bocklund bocklund deleted the MAINT-remove-Problem branch June 8, 2021 20:57
bocklund added a commit to PhasesResearchLab/ESPEI that referenced this pull request Jun 8, 2021
bocklund added a commit to bocklund/pycalphad that referenced this pull request Jun 8, 2021
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.

None yet

2 participants