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] pt space pte solver #453

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

[WIP] pt space pte solver #453

wants to merge 19 commits into from

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Jan 8, 2025

PR Summary

This is a "working" version of the PT-space PTE solver that @dholladay00 started in MR #422, and this MR should replace that one. This version compiles and runs, although I haven't carefully checked for correctness yet. (It reports it converged at least!)

Important notes:

  • This depends on MR Make sure that DensityEnergyFromPressureTemperature works for all equations of state #449 , where I make DensityEnergyFromPressureTemperature more robust across our EOS models and also add introspection necessary to bound the line search used in the PTE solver.
  • The formulation that @jhp-lanl and @dholladay00 were working on, I believe, used analytic relations to compute derivatives with respect to pressure and temperature. We should potentially pursue this, but as a first pass, I just used finite differences with DensityEnergyFromPressureTemperature to build the Jacobian.

Remaining to do:

  • Before merge, I need to do more careful correctness tests.
  • I should also do some optimization. For example, the matrix inverse for the Jacobian is still calling the QR decomposition. This is not necessary. We can just analytically invert a 2x2 by hand.
  • And also the diff will be much easier to read after Make sure that DensityEnergyFromPressureTemperature works for all equations of state #449 goes in.
  • I would like to upload @jhp-lanl 's notes to the PTE solver docs. That said I wrote down the barest of information in the docs on this formulation.

That said, this is ready for feedback.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file.
  • LANL employees: make sure tests pass both on the github CI and on the Darwin CI

If preparing for a new release, in addition please check the following:

  • Update the version in cmake.
  • Move the changes in the CHANGELOG.md file under a new header for the new release, and reset the categories.
  • Ensure that any when='@main' dependencies are updated to the release version in the package.py

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Jan 8, 2025

Some quick comments before I review (after the other MR goes in):

The formulation that @jhp-lanl and @dholladay00 were working on, I believe, used analytic relations to compute derivatives with respect to pressure and temperature. We should potentially pursue this, but as a first pass, I just used finite differences with DensityEnergyFromPressureTemperature to build the Jacobian.

I think that's a good idea. My thinking was to use the existing EOS introspection to figure out what kind of lookup should be used and then avoid any costly iterative inversion steps. But this is an optimization step and we should only pursue it if there's a clear performance need.

I should also do some optimization. For example, the matrix inverse for the Jacobian is still calling the QR decomposition. This is not necessary. We can just analytically invert a 2x2 by hand.

I agree. Echoing my previous comment, I'm glad you avoided prematurely optimizing. Let's find the bottlenecks first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants