JCSDA-internal / eva

Evaluation and Verification of the Analysis
Apache License 2.0
5 stars 12 forks source link

Adding new reader and lat/lon match transform #184

Closed asewnath closed 4 months ago

asewnath commented 5 months ago

We are adding a new reader to handle geoval files, but with the idea that it can be generalized to other types of files. A new latlon match transform was added using some work that @danholdaway had developed.

List of changes:

Resolves #177

danholdaway commented 5 months ago

Thanks for adding more capability to Eva @asewnath. I'm looking at the test files that you added and to me they look a lot like the files we already read with other classes, either ioda_obs_space or gsi_obs_space. Rather than adding a new class for reading them can we make use of those classes instead or was there something missing from them? If we do need a new class I would advocate for more descriptive name so indicate the scope of what it can read.

asewnath commented 5 months ago

Hi @danholdaway, the test file using the new reader is the geoval file. Since this geoval file has a different format compared to ioda and gsi files (along with level dimensions to consider), a new reader was created for it. In the issue mentioned in this PR, @CoryMartin-NOAA suggested that I generalize the new reader’s name. This is to use the reader for other types of similarly structured files later on.

The obs file test file I have added is read by an existing reader. This is primarily to extract lat/lon fields to be used by the match transform later on.

danholdaway commented 5 months ago

How straightforward (or not) would it be to allow the generalization of the gsi_obs_space reader to allow to levels in the variables? In my mind we've designed Eva to have specific readers with generic transforms and plotting; the reason being that plotting and transforms are naturally quite generic whereas readers tend to be naturally quite specific and dependent on the files. However there are a limited number of formats that really need to be read. Trying to write generic readers will (I suspect) result in code with lots of options and different YAML keys that will make them confusing and difficult to document. Further it will mean that you have to pass lots of arguments and so they wont lend themselves well to the APIs used in interactive mode. I think you should be able to pass a filename only and be able to read a file. Rather than pass a filename as well as information about coordinate names etc. Does that make sense or am I missing the point here?

So I would lean towards generalizing the gsi reader which is already geared towards this file format. If we really want to add another reader to handle gsi_obs_space files that have GeoVaLs that's fine with me but I would still advocate for giving it a specific name and keeping it's scope quite narrow with minimal YAML so it's in keeping with the design of the readers in general.

asewnath commented 5 months ago

Okay, I can look into generalizing the gsi_obs_space reader

CoryMartin-NOAA commented 5 months ago

@danholdaway @asewnath I think the gsi_obs_reader will be quite different from this geoval reader. Mainly because of the code put in there to do channel subsetting for the radiance observations. I do agree though that if it is a lot of work to generalize then it is probably not worth it.

asewnath commented 5 months ago

@danholdaway @CoryMartin-NOAA Then maybe we can just make the new reader name more geoval specific for now?

CoryMartin-NOAA commented 5 months ago

I'm fine with that

danholdaway commented 5 months ago

OK, that works.