noaa-oar-arl / monetio

The Model and ObservatioN Evaluation Tool I/O package
https://monetio.readthedocs.io
MIT License
17 stars 30 forks source link

fix lat and lon in _cesm_fv_reader #181

Closed blychs closed 2 months ago

blychs commented 3 months ago

Correct _cesm_fv_mm.py reader for good functionality with MELODIES-MONET

Delete variables to avoid duplication in melodies-monet
Add (optional) variables for vertical coordinates:
    temperature_k
    alt_msl_m_mid
    alt_agl_m_mid
    surfpres_pa
    pres_pa_mid

PS: Sorry for submitting 2 PRs on the same day, these are what I had finished and had not submitted yet. I think that they should be tested separately.

xref: https://github.com/NOAA-CSL/MELODIES-MONET/issues/261

zmoon commented 3 months ago

Also maybe we can set up a test using https://csl.noaa.gov/groups/csl4/modeldata/melodies-monet/data/example_model_data/cesmfv_example/CAM_chem_merra2_FCSD_1deg_QFED_world_201909-01-09_small_sfc.nc . @blychs do you want to give that a try?

blychs commented 3 months ago

Done with the .coords issue you commented. I already tested it with camchem.ipynb and it works fine.

@zmoon , I am happy to add the test. I guess it should be based on test_raqms.py ? I am unaware of what dp_pa (in test_raqms) should be, my current code is not adding that for the vertical coordinate. Is it a pressure delta? Is it needed? It is not in the wrf-chem reader, which is the one I based the vertical coordinate from.

zmoon commented 3 months ago

based on test_raqms.py ?

I would base it on the TROPOMI L2 one, since that has code to download a file from the CSL website that you can adapt. You can also look at the RAQMS one for testing inspiration, but the setup for the RAQMS tests is simpler since it uses a file that is part of the repository.

And yes I believe dp_pa is the layer thickness in Pa. But I don't think it's required to have it.

blychs commented 3 months ago

Hi Zach. The file that you suggested only has surface information. I can create tests for that, but if there is a file with vertical information available from CSL, we could do a more thorough, 3D test. Otherwise, I could probably provide one. Let me know if that's needed

zmoon commented 3 months ago

could probably provide one

Do you have one that is 200 MB or less? Or that you could make so[^1]? The "small" one at https://csl.noaa.gov/groups/csl4/modeldata/melodies-monet/data/example_model_data/cesmfv_example/ is still big enough that I would be hesitant to have it automatically downloaded.

[^1]: By time selection, spatially subsetting (horiz and/or vert), removing variables, applying NCO lossy compression, etc.

blychs commented 3 months ago

The easiest way is to subsample CSL's example to have less, which could do the trick. However, looking at that file, I realize that there are a couple of things I could do better: a) I am assuming that PMID is there, which is not always the case. I will add the calculation of the midlayer pressure using the surface, P0, hyam and hyab if PMID is not there. b) I assumed that the potential height Z3 is there, which is not always the case. I will add an optional calculation using the hydrostatic equation if Z3 is not there (CAM-chem FV is hydrostatic anyway). I could raise a warning to let the user know, just so as to future-proof the code in case a non-hydrostatic FV core is developed, but I think that the future is actually moving away from FV rather that trying to change it.

blychs commented 3 months ago

@zmoon I added a test for the surface for now. However, it is my first time working with pytest, and I am not really sure if I know what I'm doing. Feel free to strongly criticize it. Other than that, the _cesm_fv.py reader should be ready, with Z3 and PMID calculations if missing.

zmoon commented 3 months ago

Looks like the test env needs pyresample. You can add it between pyhdf and requests in environment-dev.yml.

blychs commented 3 months ago

I'm sorry for these new bugs. I should be able to have all of that fixed today.

blychs commented 3 months ago

@zmoon I think that it should be ready now

zmoon commented 3 months ago

Nice job getting pre-commit set up. codespell, however, doesn't like pres, thinks it a misspelling of press. Solution is to change the variable name or copy offending lines into the .codespell-exclude file.

blychs commented 3 months ago

I see no reason not to call it press.

blychs commented 3 months ago

@zmoon Is there still anything missing/ bug?

zmoon commented 3 months ago

@blychs there is one unresolved convo you could address. Also, I know you were interested in testing the vertical stuff, were you able to come up with a new test file to use for that?

blychs commented 3 months ago

@zmoon I have not, I could get a simple one by just removing timesteps from the run you mentioned. Would that be acceptable?

zmoon commented 3 months ago

Sure that sounds fine with me.

blychs commented 2 months ago

@zmoon I created the file already (sorry for the delay, it's in Casper already) but realized that the original file does not have the temperature... which makes calculating the hydrostatic height impossible. If you have any file with the Temperature, I could recreate that. Otherwise, let me know and I will try to get it from someone at NCAR.

zmoon commented 2 months ago

If you have any file with the Temperature

I do not, sorry. If you don't find one easily, we add that test another time.

blychs commented 2 months ago

If you have any file with the Temperature

I do not, sorry. If you don't find one easily, we add that test another time.

I'd say we go for that. I can add a warning if surface_only=False, with something like "The 3D capabilities of the cesm_fv reader are still experimental and have not been properly tested. User discretion is adviced." We currently have an approved version that doesn't work with MELODIES-MONET, so merging this would still be an improvement. As soon as I get a proper test_case, I'll add it in and the warning can be removed.