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

Add xarray backends for fgmax and fgout gridded formats #611

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kbarnhart
Copy link

@rjleveque, @mandli -

Here is a PR that addresses #598.

It may not be the most efficient (I flipud and transpose each array to put them in the orientation that xarray wants), but I find that it works.

I've tested the fgmax method on dclaw and geoclaw (the chile 2010 fgout-fgmax example) with each option for the number of variables written.

I've tested the fgout method with dclaw and geoclaw for both binary and ascii type files.

One point of discussion is whether you like the variable names I've use for fgmax (they are not exactly the same as in the fgmax object (e.g., I use h_max instead of h). I'm happy to change them to be identical if that is important to either of you.

I added an example usage script in the chile2010 fgout-fgmax example as it seemed the most sensible place to put an example.

Happy to improve this as you see fit.

@rjleveque
Copy link
Member

Thanks @kbarnhart for providing this! I haven't experimented with it much yet, but a few comments...

  1. I had to install rioxarray, which is not part of xarray.
  2. Nice that for binary .b files it seems to work fine regardless of whether binary32 or binary64 is used.
  3. When reading fgmax arrays, it seems like the topography B field should also be returned by default. This is often needed in processing the results (e.g. to plot h_max only on shore).

Eventually it would be nice to integrate this better with fgout_tools.py and fgmax_tools.py so that the xarray datasets could be worked with in the same way as the ndarray option currently supported.

I hope to revamp the fgmax output options in the near future, e.g. to allow binary output and to make it easier to specify exactly what variables should be monitored. We are also working on improving the arrival time monitoring to keep track of various different times, which we have found necessary for tsunami studies in regions of vertical land motion and for maritime modeling where drawdown is important as well as increases in depth. That might be a good time to rethink a lot of the interface and try to better coordinate these tools.

But for now it seems fine to merge this in, perhaps with some tweaks, so that it can be used for your purposes.

@mandli and @ketch: I note that our new attempt at CI throws an error for this PR in the setup step:

cd geoclaw
git checkout refs/pull/611/merge
error: pathspec 'refs/pull/611/merge' did not match any file(s) known to git

Not sure if this is something to investigate?

@kbarnhart
Copy link
Author

@rjleveque thanks for your comments. I'll make some updates to address them and ping you here when they are ready to consider.

happy to be involved in integrating these better.

@mandli
Copy link
Member

mandli commented Jun 2, 2024

Would the file xarray_backends.py be the location where any xarray backend would go? For example would we stick a gauge xarray backend in there?

@kbarnhart
Copy link
Author

@mandli - I thought a tiny amount about this and it seemed to me like there were two options. Either all the xarray backends go in one module or each xarray backend goes inside a module associated with the relevant clawpack data structure (e.g., the FGOutBackend goes in fgout_tools.py).

I have a slight preference towards the latter option. However, I implemented the former because I didn't want to make the fgout_tools and fgmax_tools files longer and make it hard to discover the backends.

All of this is to say that I'm not sure the best place for these within clawpack.geoclaw.

@mandli
Copy link
Member

mandli commented Jun 4, 2024

@kbarnhart Those were the two options that came to my mind as well. Given that gauges are not actually handled here in GeoClaw but in PyClaw (it's common across most of the Clawpack packages) it may make more sense to keep them in the associated data structures. Similarly we could/should do something with other data formats, such as friction from land use or storm data, for which there may be multiple backends. Should be easy to change up until we merge this fully and make a release though.

@mandli
Copy link
Member

mandli commented Jun 4, 2024

Variables when it makes sense should be named the same thing probably to reduce confusion although I do like the more descriptive variable you mentioned.

@kbarnhart
Copy link
Author

Some notes based on conversation with @rjleveque:

  • Keep the more descriptive names
  • Keep these backends in their current file
  • In addition to adding B as a variable, add level

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

3 participants