E3SM-Project / e3sm_diags

E3SM Diagnostics package
https://e3sm-project.github.io/e3sm_diags
BSD 3-Clause "New" or "Revised" License
38 stars 31 forks source link

edit in-place modified derived variables #805

Closed mahf708 closed 4 months ago

mahf708 commented 4 months ago

Description

To distinguish variables modified in-place from raw values potentially requested elsewhere in the code. This was causing a bug in that other places in the code where requesting these variables, but getting them modified by scalars. See #804.

Checklist

If applicable:

chengzhuzhang commented 4 months ago

@mahf708 I'm working on a release candidate for e3sm unified today, I'm wondering if we are able to include this fix?

By skimming this draft PR, I was trying to understand the change in v3 code since we worked on the aerosol table, does it mean that the variable being output changed its units, and also variable names?

mahf708 commented 4 months ago

It shouldn't change the aerosol budget table, but it will change the variable names of ABURDEN and Mass_ if they appear elsewhere (like in the overall table). I had it in draft mode because I didn't get a chance to test it yet...

If you are okay with testing it and releasing, then go ahead. I will try to run a quick test soon too...

chengzhuzhang commented 4 months ago

@mahf708 thank you for the update. I have another PR to merge before the release, let me know how the testing goes, or I can merge and test on main before the release.

mahf708 commented 4 months ago

Testing:

✅ fixes the aerosol budget table: https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.ngmahfouz/E3SMv3_dev/misc/test_pr_805/custom/viewer/table/run.20231110.v3b01-Gustiness-x6tP3v2.F20TR.chrysalis-ANN-budget-table.html

chengzhuzhang commented 4 months ago

Awesome! @mahf708 thank you for the quick fix!

@yunpengshan2014 thank you for reporting the error so that we can fix it in this release.

tomvothecoder commented 4 months ago

I've moved the comments from #800 that were intended for this PR below.

@chengzhuzhang:

Update: Never mind. The simulation doesn't have all the variables needed for the table. Sorry for the false alarm. @mahf708 in my test, I ran into one problem with aerosol budget table:

2024-04-12 13:42:45,788 [INFO]: aerosol_budget_driver.py(run_diag:147) >> Variable: bc
2024-04-12 13:42:45,877 [ERROR]: core_parameter.py(_run_diag:269) >> Error in e3sm_diags.driver.aerosol_budget_driver
Traceback (most recent call last):
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/parameter/core_parameter.py", line 266, in _run_diag
    single_result = module.run_diag(self)
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/driver/aerosol_budget_driver.py", line 148, in run_diag
    metrics_dict_test[aerosol] = generate_metrics_dic(
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/driver/aerosol_budget_driver.py", line 82, in generate_metrics_dic
    elvemis = data.get_climo_variable(f"{aerosol}_CLXF", season)
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/driver/utils/dataset.py", line 161, in get_climo_variable
    variables = self._get_climo_var(filename, *args, **kwargs)
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/driver/utils/dataset.py", line 386, in _get_climo_var
    derived_var = func(*variables)
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/derivations/acme.py", line 1654, in <lambda>
    (("bc_a?_CLXF",), lambda *x: molec_convert_units(sum(x), 12.0)),
  File "/home/ac.zhang40/y/envs/edv2110/lib/python3.10/site-packages/e3sm_diags/derivations/acme.py", line 175, in molec_convert_units
    if var.units == "molec/cm2/s":
AttributeError: 'int' object has no attribute 'units'

I'm wondering if this is caused by some new change of the output? I'm testing with v3.LR.historical run: /lcrc/group/e3sm2/ac.zhang40/E3SMv3/v3.LR.historical_edv2110/post/atm/180x360_aave/clim/30yr

@mahf708

Yes, this is in earlier code. It still is concerning (though I don't think it is wrong or blocking to have this behavior). We can add a fix to avoid these errors, I would have hoped the logic wouldn't reach the units check in these cases...

@chengzhuzhang

Agreed. I think I commented on the wrong PR, the conversation should go to #805.. Sorry, Tom.