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

Initial guess for temperature #310

Open
MTCam opened this issue Apr 7, 2021 · 6 comments
Open

Initial guess for temperature #310

MTCam opened this issue Apr 7, 2021 · 6 comments

Comments

@MTCam
Copy link
Member

MTCam commented Apr 7, 2021

This issue is intended to continue a conversation about how to handle the initial temperature guess that we pass into pyro.get_temperature in Pyrometheus-based EOS in MIRGE-Com.

Currently, if we do not have a reasonable guess, get_temperature takes many iterations, and can sometimes apparently get the wrong answer.

Current approach: just pass a constant default (e.g. 300K) (expensive, potentially wrong)
Suggested: pass the temperature from previous step (ideal, but tricky, requires restarting temperature else we get a different temperature upon restart!)
Suggested: pass a deterministic guess that we compute based on current state (promising, but tricky to craft the routine for a deterministic guess)

The ultimate plan is to use the temperature from the previous step. Here is the current plan to do that (as I understand it) advised by @inducer:

  • Guide/plan:
    • Plan to use the (de)serialization interface to detect a memoized temperature and paste it onto the resulting CV object
    • serialize and deserialize operations deconstruct and reconstruct a data-containing object before and after (e.g. arithmetic) operations
    • Array container objects get a default implementations of the (de)serialization functions
    • requires non-default implementation of deserialize_container (and maybe serialize_container, too) for CV which will reconstruct a new CV out of a previously serialized CV.
  • References:
  • Questions:
    • Seems we need to implement both deserialize and serialize in order to capture the temperature; is this true?
    • It is unclear to me how to capture the temperature in a way that does not try to make it participate in the operation requiring (de)serialization. Should we be concerned with this?
    • Once the old temp is captured from one of the arguments to an operation, how/where to store it between the serialization and deserialization operations?
    • Can one still call the default implementations of the (de)serialize functions from the new implementations? If not, how do we make sure to implement (de)serialization operations with correct behavior?
    • How can I discover (and copy) the default (de)serialize implementation for CV?
    • Should the temperatures (old and new) be stored frozen or thawed?
  • Potentially troublesome:
    • Once this works, we still need to make sure we can restart temperature - this could be done fairly easily I think by storing temperature field in restart file and using it as the initial guess at first step, but I could be missing something.

@inducer, @ecisneros8, @anderson2981

@MTCam
Copy link
Member Author

MTCam commented Apr 7, 2021

As @inducer suggested in #236, we may need to beef up the interface to allow better management of the guesses. How we handle guess management will be intimately connected with the general strategy we plan to pursue.

@MTCam
Copy link
Member Author

MTCam commented Oct 27, 2021

An alternate solution might be something along the lines of #534. The advantage here is that it is very easy to follow and understand, and currently works to solve the issue even through restart.

@inducer
Copy link
Contributor

inducer commented Oct 27, 2021

The main unsolved issue IMO is how we get the lazy eval infrastructure to enumerate this as an input. #534 does nothing in that regard. See inducer/arraycontext#101 for infrastructure discussion on that.

I also really do not like the blatantly stateful nature of that proposed change.

@MTCam
Copy link
Member Author

MTCam commented Oct 28, 2021

The main unsolved issue IMO is how we get the lazy eval infrastructure to enumerate this as an input. #534 does nothing in that regard. See inducer/arraycontext#101 for infrastructure discussion on that.

I also really do not like the blatantly stateful nature of that proposed change.

I sensed that #534 would run into a lot of resistance. However, it simply illustrates what we are trying to do, and is adjacent to what application folks are thinking about when having a hard time understanding why using the temperature from the previous step needs to be so complicated.

In your current plan, the thing that we want to work (plz correct if I'm off-base here) I think is this:

new_state = state + state_update,

where state is the CV object with a memoized temperature on it, state_update is a CV object with the RHS (but no temperature field memoized onto it), and new_state will save state's version of temperature as the old_temperature for later use by the EOS when computing the new temperature for new_state.

There may be enough nuance here (e.g. one state has it; one state doesn't, new state needs to store as a different field, etc) to preclude handling this in-general down in the array container guts. The capability developer will need some detailed control over how the non-participatory fields are propagated forward through operators.

@inducer
Copy link
Contributor

inducer commented Oct 28, 2021

why using the temperature from the previous step needs to be so complicated.

Think about it from this angle: In order to "compile" an array expression, we need to be able to enumerate all inputs to it. Everything that's not explicitly an argument (i.e. an input) to the function being compiled is treated as "constant data", i.e. it is assumed to not change between function evaluations. That's a fair assumption for things like differentiation matrices or Jacobians, but not a fair assumption for this temperature initial guess.

An easy solution is to simply pass the temperature initial guess as an explicit additional input to the RHS, but then this changes lots of interfaces (time steppers etc.) that shouldn't need to change.

Hence my thought to "ship this along" in the solution container.

@MTCam
Copy link
Member Author

MTCam commented Oct 29, 2021

why using the temperature from the previous step needs to be so complicated.

Think about it from this angle: In order to "compile" an array expression, we need to be able to enumerate all inputs to it. Everything that's not explicitly an argument (i.e. an input) to the function being compiled is treated as "constant data", i.e. it is assumed to not change between function evaluations. That's a fair assumption for things like differentiation matrices or Jacobians, but not a fair assumption for this temperature initial guess.

Thanks! This explanation and @kaushikcfd's similar one are very helpful.

An easy solution is to simply pass the temperature initial guess as an explicit additional input to the RHS, but then this changes lots of interfaces (time steppers etc.) that shouldn't need to change.

Hence my thought to "ship this along" in the solution container.

Please have a fresh look at #534 when you have a chance. After @kaushikcfd and I discussed at length, we are trying a reference state approach where we are just passing the state at the beginning of the timestep to the RHS to do with what it will. In our case, we feed it to the EOS which uses the already-memoized temperature on that state to seed Newton.

@MTCam MTCam mentioned this issue Dec 2, 2021
8 tasks
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 a pull request may close this issue.

2 participants