217 teaching `fre cmor` to bite #218

ilaflott · 2024-10-24T17:10:54Z

this branches off of #205

gfdl-only specialty tests progress, the calls of which are seen in run_test_file_cases.py

code QA:

I ran my code
I tried to make my code readable
I tried to comment my code
I wrote a new test, if applicable
I wrote new instructions/documentation, if applicable
I ran pytest and inspected it's output
I ran pylint and attempted to implement some of it's feedback

This PR adds...

fix for tmp directory + filename handling
fix: cmor_lev no longer referenced before assignment
doc string for rewrite_netcdf_vars
a currently one-off gfdl-only testing script, run_test_file_cases.py outside of the pytest framework to avoid making the repo explode in size
updates the directory README accordingly
updates the click interface help menu read outs

…in new optional argument for additional control over file targeting. we also now have a \"run one file only\" style flag. new test config file fre/tests/test_files/CMORbite_var_list.json contains variables for processing test cases being workd through in run_test_file_cases.py with notes

…ariable8-grids

…e code currently relies on. unsure how useful the exact way i wrote things down is, but im hoping ot leverage it for structure and or setting up some formulaic approaches and or rules. additionally, merge multiline printstateaments into one call using legal but maybe awkward looking multiline syntax, some other readability edits too

…see how it goes...

ilaflott · 2024-10-25T18:53:19Z

Pausing to ponder for a second- the following two commits are really about a different problem.

commit 31996b08b3c2ab3c5510a86606c01ef305383a55 (HEAD -> 217-the-cmorbite-one-variable8-grids, 
    fix undefined variable name errors

commit 4b313384a8c0cda3bb4eab2d398d99d3ada8a93c
    alrightly, lets move some of this hardcoding to the top of the file, see how it goes...

I could branch that development off here and revert the commits to make this less about exposing configuration and more about accommodating a wider array of input cases.

ilaflott · 2024-10-28T16:00:27Z

spinning off branch fre-cmor-config-exposed at commit
31996b08b3c2ab3c5510a86606c01ef305383a55 from this PR. Then, this PR's branch will have the aforementioned commit, 4b313384a8c0cda3bb4eab2d398d99d3ada8a93c, and possibly parts of 62852833ecc7f42c3fbae1433fbaf341bba469cb reverted, as those edits are really attempting to do something worthwhile (making hard-coded dependencies more obvious) but not related to this PR's primary goal.

for the exposed-config branch, see draft PR #227

…bite-one-variable8-grids

This reverts commit 31996b0.

…e file, see how it goes..." This reverts commit 4b31338.

… for now

ilaflott · 2024-10-28T21:52:25Z

as of 434dc3e, 4 out of the 9 cases in run_test_file_cases work outright. of the five failures, two are due to the lack of a lon/lat/both coordinates, two are actually associated with the copy_nc routine, and the last is associated with 4-dimensional land-use data where one has the typical horizontal coordinates, time, and NOT a z-coordinate

…tions at the cli. update test file cases for addressing no lat/lon case(s)

ilaflott · 2024-10-29T18:16:32Z

forwarding some good PR #205 feedback here and below...

that PR getting too big though...

ilaflott · 2024-10-29T18:33:19Z

from PR #205 feedback "do we really need to copy the input file before CMORizing?" - @ceblanton

i don't particularly think we need to, maybe batch related? --> perhaps batch configuration option... or check

ilaflott · 2024-10-29T18:34:17Z

from PR #205 feedback - regarding copy_nc, "If the input file is NC3 will the output file be NC3 or NC4"

need to consider file-format preservation/specification

ilaflott · 2024-10-29T18:35:17Z

from PR #205 feedback - regarding create_tmp_dir, the logic of this function should be clarified or revisited

ilaflott · 2024-10-29T18:37:22Z

from PR #205 feedback

generally: we should be using a variable's "bounds" attributes to pull in assoc coordinate data. This came up discussing time axes, but might as well look at lat and lon or other coordinates similary.

… checks, trying to generally allow this code to be more flexible. not currently working atm

…ns. none checks, trying to generally allow this code to be more flexible. not currently working atm" This reverts commit d80b790.

ilaflott · 2024-10-31T19:26:52Z

next PR #237 will address above todo cases 4, 6, 7, 8, and 9.

ceblanton

Looks great, Ian. I didn't run it, but with your test cases I believe you, and we'll have plenty more opportunities in the future to run it.

I was slightly confused about the optional "local var" setting, mostly wondering why it's not required and normally set to the "target" variable name. But later on the "local var" IS required, so I think I just confused myself.

And then the question about whether the dimension bounds should be retrieved in a CF-friendly way, but regardless it's a fair assumption.

Aside from those two probable non-issues, all of the changes are clear improvements for functionality and clarity. Onward!

ceblanton · 2024-10-31T20:21:40Z

fre/cmor/README.md

+                           process only filenames matching that variable name.
+                           I.e., this string help target local_vars, not      
+                           target_vars.                                       
+  --help                   Show this message and exit.                        


I can't ever really disagree with documentation, and don't disagree with this, but we'll have to keep this up-to-date with the actual --help output content which is defined elsewhere. Eventually, documentation like this will be auto-created through sphinx somehow.

I've been pondering this too. hear me out for a second- I know that the way (edit) ~~we generate~~ i generated coverage badges in our pipeline is a bit hacky- utilizing the deployed github pages to host the generated image at a static URL for linking to the README.

But... there DO exist workflows that edit the coverage badge in the repo directly and makes the commit itself- i tried to make those work but couldn't figure it out on a reasonable timeline.

Why do i bring this up? Well, in theory if a workflow could commit to the repo for a generated test-coverage badge, it should also be able to commit a quick README edit based on the latest fre <tool> --help or fre <tool> <subtool> --help. That's tech debt worth noting down for future payment.

fre/cmor/cmor_mixer.py

ceblanton · 2024-10-31T20:25:24Z

fre/cmor/cmor_mixer.py

-        print('(cmorize_target_var_files) NOT using /local /work /net (tmp_dir = outdir/tmp/ )')
-        tmp_dir = f"{outdir}/tmp/"
+        print(f'(create_tmp_dir) NOT using /local /work /net (tmp_dir = {outdir}/tmp/ )')
+        tmp_dir = str( Path(f"{outdir}/tmp/").resolve() )


we agreed to revisit this tmpdir and if it's needed at all. It seems odd to combine the string and path type conversion in one line but I've had to do it too.

see #240 ! but perhaps we should discuss the best way to use/handle pathlib.Path in our codebase... lest we get too deep in the hole!

fre/cmor/cmor_mixer.py

ceblanton · 2024-10-31T20:34:02Z

fre/cmor/cmor_mixer.py

+                cmor_lev = cmor.axis( "alternate_hybrid_sigma",
+                                      coord_vals  = lev[:],
+                                      units       = lev.units,
+                                      cell_bounds = ds[vert_dim+"_bnds"] )


Is this line creating the cell bounds from scratch or do they have to be in the input file?

If it's creating it from thin air, then this should be fine. But if it has to be in the input file, then perhaps it is named something different, but is probably vert_dim+'_bnds'. The CF way to find it would be similar to the dimension axis:

double lev(lev) ; lev:units = "1.0" ; lev:long_name = "hybrid sigma pressure coordinate" ; lev:axis = "Z" ; lev:positive = "down" ; lev:formula = "p(n,k,j,i) = ap(k) + b(k)*ps(n,j,i)" ; lev:formula_terms = "ap: ap b: b ps: ps" ; lev:bounds = "lev_bnds" ; lev:standard_name = "atmosphere_hybrid_sigma_pressure_coordinate" ;

ds is grabbed as a netCDF4.Dataset from the targeted files in the input directory, so ds[vert_dim+'_bnds'] demands a value pointed to by key vert_dim + '_bnds' exists in ds.

ceblanton · 2024-11-05T16:15:32Z

fre/cmor/cmor_mixer.py

+INPUT_SUBCASE2_1_ZFACT_BNDS = ['ap_bnds','b_bnds']
+#-
+OUTPUT_SUBCASE2_0_ZFACT_VAL_NAMES = ['ap','b']
+OUTPUT_SUBCASE2_0_VERT_LVL_NAME = 'altername_hybrid_sigma'


Completely agree with this approach. It's far better than hard-coding in actual code but retains flexibility and transparency.

ilaflott linked an issue Oct 24, 2024 that may be closed by this pull request

CMORbite: success across different variables and tables #217

Closed

15 tasks

quick for two commits Merge branch 'main' into 217-the-cmorbite-one-v…

015c601

…ariable8-grids

ilaflott self-assigned this Oct 24, 2024

having run all test cases, a theme of failures is starting to emerge...

493c4dd

ilaflott changed the title ~~217 the cmorbite one variable8 grids~~ 217 teaching CMOR to bite Oct 24, 2024

ilaflott changed the title ~~217 teaching CMOR to bite~~ 217 teaching fre cmor to bite Oct 24, 2024

ilaflott added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request tricky Likely to encounter significant friction to solution priority: HIGH labels Oct 25, 2024

ilaflott added 4 commits October 25, 2024 12:24

add print statement to wrapper function for easier cli debugging calls

153a779

alrightly, lets move some of this hardcoding to the top of the file, …

4b31338

…see how it goes...

fix undefined variable name errors

31996b0

ilaflott and others added 4 commits October 28, 2024 15:16

grab recent changes, of course. Merge branch 'main' into 217-the-cmor…

f35599d

…bite-one-variable8-grids

Revert "fix undefined variable name errors"

ff6de5b

This reverts commit 31996b0.

Revert "alrightly, lets move some of this hardcoding to the top of th…

515d56c

…e file, see how it goes..." This reverts commit 4b31338.

bug fix for a case or two, tidy up the presentation of the test cases…

434dc3e

… for now

update readme and frecmor.py with better doc strings and help descrip…

14b08cd

…tions at the cli. update test file cases for addressing no lat/lon case(s)

ilaflott mentioned this pull request Oct 29, 2024

cmor tool improvements #205

Merged

7 tasks

ilaflott added 4 commits October 29, 2024 14:54

intermediate commit- new function to compar variable dimensions. none…

d80b790

… checks, trying to generally allow this code to be more flexible. not currently working atm

Merge branch 'main' into 217-the-cmorbite-one-variable8-grids

2f84555

Revert "intermediate commit- new function to compar variable dimensio…

1a15d95

…ns. none checks, trying to generally allow this code to be more flexible. not currently working atm" This reverts commit d80b790.

Merge branch 'main' into 217-the-cmorbite-one-variable8-grids

e696487

ilaflott mentioned this pull request Oct 31, 2024

Cmor bite wrapup #237

Merged

18 tasks

ilaflott requested a review from ceblanton October 31, 2024 19:28

ilaflott marked this pull request as ready for review October 31, 2024 19:28

ceblanton approved these changes Oct 31, 2024

View reviewed changes

ilaflott merged commit 16af470 into main Nov 1, 2024
2 checks passed

ilaflott deleted the 217-the-cmorbite-one-variable8-grids branch November 1, 2024 16:42

ceblanton reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

217 teaching `fre cmor` to bite #218

217 teaching `fre cmor` to bite #218

ilaflott commented Oct 24, 2024 •

edited

Loading

ilaflott commented Oct 25, 2024

ilaflott commented Oct 28, 2024

ilaflott commented Oct 28, 2024

ilaflott commented Oct 29, 2024 •

edited

Loading

ilaflott commented Oct 29, 2024

ilaflott commented Oct 29, 2024

ilaflott commented Oct 29, 2024

ilaflott commented Oct 29, 2024

ilaflott commented Oct 31, 2024

ceblanton left a comment

ceblanton Oct 31, 2024

ilaflott Nov 1, 2024 •

edited

Loading

ceblanton Oct 31, 2024

ilaflott Nov 1, 2024

ceblanton Oct 31, 2024

ilaflott Nov 1, 2024

ilaflott Nov 1, 2024

ceblanton Nov 5, 2024

217 teaching fre cmor to bite #218

217 teaching fre cmor to bite #218

Conversation

ilaflott commented Oct 24, 2024 • edited Loading

ilaflott commented Oct 25, 2024

ilaflott commented Oct 28, 2024

ilaflott commented Oct 28, 2024

ilaflott commented Oct 29, 2024 • edited Loading

ilaflott commented Oct 29, 2024

ilaflott commented Oct 29, 2024

ilaflott commented Oct 29, 2024

ilaflott commented Oct 29, 2024

ilaflott commented Oct 31, 2024

ceblanton left a comment

Choose a reason for hiding this comment

ceblanton Oct 31, 2024

Choose a reason for hiding this comment

ilaflott Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

ceblanton Oct 31, 2024

Choose a reason for hiding this comment

ilaflott Nov 1, 2024

Choose a reason for hiding this comment

ceblanton Oct 31, 2024

Choose a reason for hiding this comment

ilaflott Nov 1, 2024

Choose a reason for hiding this comment

ilaflott Nov 1, 2024

Choose a reason for hiding this comment

ceblanton Nov 5, 2024

Choose a reason for hiding this comment

217 teaching `fre cmor` to bite #218

217 teaching `fre cmor` to bite #218

ilaflott commented Oct 24, 2024 •

edited

Loading

ilaflott commented Oct 29, 2024 •

edited

Loading

ilaflott Nov 1, 2024 •

edited

Loading