Pressio / pressio

core C++ library
Other
45 stars 7 forks source link

Confirm consistent usage of getReferenceToJacobian #108

Closed jtencer closed 4 years ago

jtencer commented 4 years ago

If the Jacobian (decoder not ODE) is not constant, we need to confirm that the correct Jacobian is used in all cases. This would also enable a time-varying Jacobian.

fnrizzi commented 4 years ago

@jtencer can you please list here what are the rules in your view that one should check for making sure the "correct jacobian" is used?

jtencer commented 4 years ago

Currently, the user defined mapping interface includes getReferenceToJacobian() which is called whenever the Jacobian is queried. To support nonlinear mappings and time-varying Jacobians, we need to:

  1. Confirm that there is no scenario when the Jacobian could need to be updated in between calls to getReferenceToJacobian
  2. Include the ROM state as an argument to getReferenceToJacobian i.e. jacobian_type & getReferenceToJacobain(const rom_state_type & romState)
  3. Confirm that getReferenceToJacobian is only called once per rom state update to avoid unnecessary updates.

Because 2 has not been completed, my hacky implementation currently updates the Jacobian as part of applyMapping which results in unnecessary updates.

fnrizzi commented 4 years ago

@jtencer @eparish1 @pjb236

Ok, let's go in order, since this is important:

jtencer commented 4 years ago

In the regression test I have void updateJacobian(const rom_state_type & romState) const

My thought was that we would call that method from getReferenceToJacobian rather than having Pressio call it separately. This was the reason I wanted to avoid spurious calls to getReferenceToJacobian. If Pressio calls updateJacobian separately, then additional calls to getReferenceToJacobian aren't a big deal because there would be no computation involved in that op.

I think it's appropriate to only update the Jacobian once per full iteration. I think that if the Jacobian is changing significantly within an iteration, that either the step is too large or the manifold is too crinkly.

fnrizzi commented 4 years ago

@jtencer @eparish1 @pjb236

jtencer commented 4 years ago

I think we need to write out the equations we’re solving and figure out at what level we’re ok with linearization. That should be what governs the frequency we update the Jacobians.

fnrizzi commented 4 years ago

Yes, makes sense... would you be able, please, to start this? and then we can follow up here. This being said, I will not include this in the new upcoming release ok? :)

jtencer commented 4 years ago

I think we need to call computeJacobian each time we compute the ROM Jacobian. I don't think we need to recompute the old steps though.

fnrizzi commented 4 years ago

This relates to #107

I think we need to call computeJacobian each time we compute the ROM Jacobian. I don't think we need to recompute the old steps though.

Ok. So why don't we need to recompute old steps? can you post a pdf here with the math you did to figure this out?

jtencer commented 4 years ago

Because the appropriate decoder Jacobian for computing the old steps is the one that's evaluated at the old step. The time integrator shouldn't need to be aware of anything besides the history of states and the method for computing the next state given the prior states. What would be the justification for changing prior state information based on the current state?

fnrizzi commented 4 years ago

What would be the justification for changing prior state information based on the current state?

None! I agree with you! just wanted to make sure :) I will work on it for the new release

fnrizzi commented 4 years ago

@jtencer @eparish1 @pjb236 I am working on this and all is fine for explicit Galerkin.
But came across a doubt when working on implicit Galerkin with the discrete-time API (but I think the same happens with LSPG). Let's say we are a at a newton step k and let's call the decoder's jacobian J_d.

If we update the decoder's Jacobian ONLY when we compute the ROM Jacobian, then we have a discrepancy because at k the ROM residual computation will use the previous J_d while the computation of the ROM Jacobian will use the new J_d. So shouldn't we update the decoder's Jacobian once at the beginning of every solver's step?

pjb236 commented 4 years ago

I don't see a way around that if we want to be mathematically correct. J_d should be computed whenever we update the state. However, updating J_d for every Newton step could be very costly.

fnrizzi commented 4 years ago

what if pressio passes you also the time step we are at? do you need the solver iteration too? that way you can decide whether to update or not. would that make sense?

  template<typename gen_coords_t>
  void updateJacobian(const gen_coords_t & genCoordinates) const
  {
  }

if you have a linear decoder, then of course the above is a noop but for a generic mapping it makes sense right?

fnrizzi commented 4 years ago