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

Source water inversion with multiple state variables #23

Closed
15 tasks done
ggebbie opened this issue Mar 27, 2023 · 5 comments · Fixed by #26
Closed
15 tasks done

Source water inversion with multiple state variables #23

ggebbie opened this issue Mar 27, 2023 · 5 comments · Fixed by #26

Comments

@ggebbie
Copy link
Owner

ggebbie commented Mar 27, 2023

  • Start from test "source water inversion: obs at one time, many surface regions, with circulation lag" or the case with an observational timeseries
  • construct a random M (water-mass matrix) like before, but construct two "x" variables
  • decide how to return the x solution: 1) 3D DimArray with time, surface region, variable type, 2) NamedTuple with 2 fields that each have time, surface region, 3) Two or more DimArrays
  • update convolve (i.e., the forward function that does x -> y) to handle the output from the previous step.
  • Store convolve output as the synthetic data. Ultimately the data should be corrupted according to Cnn.
  • Given knowledge of y and dims(x), produce a first guess x0 that has the same form as step 3 above
  • Run x0 through the forward function to make a first prediction of the observation(s) y0
  • See if datacost still works to compute half of the cost function
  • Update construction of Cxx to handle two variables, assume diagonal in first step, relax this later, probably best to use vec to change x0::Union{DimArray,NamedTuple,etc.} into a vector, Cxx requires the same type of bookkeeping, perhaps a new function to do this automatically would be useful
  • Modify controlcost to handle two state variables, must take the Cxx output from the previous step
  • If using a timeseries of observations, modify impulseresponse to take M water-mass matrix and return E the matrix used in BLUEs.solve
  • Check that E*vec(x0) gives the same thing as convolve(x0,M)
  • Pass new problem to BLUEs.solve
  • Modify BLUEs.getproperty to handle cases where there are two state variables, if the state variables are in a DimArray then ndims>2 could be checked to see if there are multiple state variables
  • Check that output and covariance is appropriately displayed in the REPL in the same form as x and x0, and that x.x contains this information
@b-r-hamilton
Copy link
Collaborator

Added testset for two state variables, one observation. I assume that the one observation is some linear combination of the two state variables propagated through the M matrix.
Some choices I made -

  • 3D DimArray for x
  • convolve method is not quite right - see comment
  • any reason not to use copy to generate x_0 template?
  • Cxx basically worked in original format...?

@ggebbie
Copy link
Owner Author

ggebbie commented Mar 30, 2023

Awesome work! Everything ran like a dream and seemed to give reasonable output.

@ggebbie
Copy link
Owner Author

ggebbie commented Mar 30, 2023

  • 3D DimArrays seem to work well, good choice. I plan to take account (i.e., make an issue) for all the little formatting issues between making a 1-element vs. 1x1-element UnitfulMatrix. Then I can fix them in UnitfulLinearAlgebra so that the code just does what we want. Thanks for handling these things which must have been annoying!

  • I saw the convolve comment in src/BLUEs.jl. I had the same issue and resorted to a conditional statement (if ndims ...) which isn't applicable when having multiple state variables. Idea: make coeffs simpler and pass it to convolve to send a signal that there are multiple state variables. coeffs::Vector could simply be the last argument of convolve. Then add a multiplication inside of convolve.

  • No problem using copy in this example. I didn't use it because I was pretending that I only knew dims(x) but not parent(x).

  • Yes! Cxx works in the original format, yet it calculates a 2x2 block matrix with the proper combination of units in each block. I think this is the way to go. Likewise, the "E" matrix was derived in a pretty snazzy and automated way. This is what I was hoping for.

  • Slight bug that I probably introduced: Cxx and Cnn should have squared sigma-x and sigma-n values on the diagonal.

  • Consider having a "show" method that gives the name of each state variable and outputs a 2D DimArray for each one.

  • Please do try the next case if you get a chance! I think this is going well and we can get this into BLUEs#main. We can collect extra stuff in BLUEs and clean it up later.

@b-r-hamilton
Copy link
Collaborator

Did the two state variables + timeseries test! Fixed the following issues, for both new tests

  • Updated convolve so that it both convolves and linearly combines for the two state variable case
  • made sure Cxx and Cnn are \sigma^2 on the diagonal, not \sigma
  • added the show method for the x variable, probably still needs some help

@b-r-hamilton
Copy link
Collaborator

Revisiting some of the initial things in Jake's list that I apparently didn't feel like dealing with 2 weeks ago

  • controlcost and datacost just work, apparently. No modification needed
  • getproperty also works for the two state variable case, returns a vector with the first dimension/state variable info on top, concatenated with the second dimension/state variable. As long as this behavior is okay, I don't think I need to modify this either.

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