Feature/viirs aod smoke dust #1550

BlakeJAllen-NOAA · 2024-09-11T07:17:15Z

Description

The VIIRS AOD Converter has been modified to add the ability to identify smoke- and dust-affected AOD observations and output them in separate IODA files. This processing uses information contained in Aerosol Data Product (ADP) files that are distributed as a companion dataset for each VIIRS AOD granule. The smoke and dust processing feature is initially needed for use with the RRFS Smoke/Dust model.

The list and description of changes to enable the new functionality is somewhat long, but I will provide it if it would be helpful for the code review.

Issue(s) addressed

N/A

Dependencies

None

Impact

Expected impact on downstream repositories:

None. Converter behavior should not change if new smoke and dust processing capability is not activated using --adp_mask CLI argument

Checklist

[ X] I have performed a self-review of my own code
[ X] I have made corresponding changes to the documentation (assuming the current documentation for the converters is only that in the README.md file)
[ X] I have run the unit tests before creating the PR

…affected observations

… for clarity. 2.) Renamed variable conf_mask in flag_confidence_lvl() to inv_conf_mask to make clear that it is the inverse of a mask rather than a mask. 3.) Placed flag_confidence_lvl() diagnostic output block in if block (not run by default) instead of leaving it in as a comment block (perhaps this diagnostic prinout block should just be removed, though).

…more general set of original AOD output filenames

* Previously the program derived the ADP filename from the AOD filename, but it assumed the files were stored in a certain directory structure * Previously, --adp_mask was a boolean CLI argument * Now ADP filename(s) specified by user via CLI argument --adp_mask=<file or files>

* Fixed variable name for ADP filenames * Added output of filenames currently being processed * Made --adp_mask filename specification accept multiple filenames (and made it produce a list of strings instead of a string)

* Argument parser help messages made clearer * Also for clarity, argument --adp_qc_lvl, variable adp_qc_lvl, and attribute AOD.adp_qc_lvl renamed to --adp_conf_lvl, adp_conf_lvl, and AOD.adp_conf_lvl, respectively * README updated with usage information for the smoke and dust processing

src/compo/viirs_aod2ioda.py

CoryMartin-NOAA · 2024-09-12T12:57:38Z

src/compo/viirs_aod2ioda.py

@@ -13,10 +13,57 @@
 import numpy as np
 import os

+import glob


this isn't used?

Thanks for catching that import, it was left over from an earlier way of creating the list of ADP files. I have removed it in the newest commit 5161a8b.

ytangnoaa · 2024-09-12T13:18:33Z

src/compo/viirs_aod2ioda.py

@@ -13,10 +13,57 @@
 import numpy as np
 import os

+import glob


this imported module is not used

Thanks for catching that import, it was left over from an earlier way of creating the list of ADP files. I have removed it in the newest commit 5161a8b.

ytangnoaa · 2024-09-12T13:28:24Z

src/compo/viirs_aod2ioda.py

+                    qcall_dust = qcall_dust[mask_thin_dust]
+                    obs_time_dust = obs_time_dust[mask_thin_dust]
+
+            # calculate and store ob errors
            # defined surface type and uncertainty
            if self.method == "nesdis":


This scripts looks only support the method of "nesdis". If it is not "nesdis", the variable "errs" become undefined. If so, please update the document mentioning that this script only support nesdis product. The zero observation error could cause some issues in assimilation.

Good point @ytangnoaa. Based on the documentation, it looks like when that method selection conditional was added there was supposed to be an else block that set the errors to zero if the nesdis method wasn't specified with -m/--method. So the current documentation is incorrect regarding the default behavior.

I could add in the missing else block to set the errors to zero, but given that both undefined and zero error values could cause problems during DA, I would propose changing the default behavior to use the nesdis method and then updating the documentation accordingly. That way the method option will remain available at the command line for existing scripts that call the code using that option, but the ability to use a method other than nesdis will be gone unless additional methods are added in later. If you think anyone may for some reason be using the converter with the expectation of it following the current default of leaving the error values undefined, though, then I will just update the documentation.

For clarity, the options I see are:
1.) Leave the code as is, update the documentation to note that the default behavior leaves the error values undefined and is not recommended.
2.) Add in the else block to set the error values to zero for the default case to match the behavior stated in the current documentation, but also update the documentation to note that the default zero error values may cause problems.
3.) Change the default value for the -m/--method argument from none to nesdis and update the documentation to note that the nesdis method is the only method currently available, but the argument remains available for ease of possible future expansion (and for current backward compatibility).

Thank you for this information. Since this script is only tested with the nesdis product, the option 3) looks more reasonable

We should have a default option that do make sense and should avoid creating strictly 0 errors. This WILL cause problems in data assimilation. The data assimilaiton code will crash. So we should modify the code (the if statement in question here) to avoid creating zeros as errors and make sure that we can only use the nesdis option or whatever allowed options we could have in the future.
Ok with solution 3. And update the the help argument to something that is not confusing: for example: obs error calculation method available: nesdis, user must specify it or the converter won't work.
Thanks

simplify code for changing case of conf_lvl in function flag_by_confidence_lvl Co-authored-by: Cory Martin <[email protected]>

jeromebarre

Please add a test for this new capability.

jeromebarre · 2024-09-12T19:30:53Z

src/compo/viirs_aod2ioda.py

+                print('Processing the following AOD and ADP files:')
+                print(f)
+                print(self.adp_mask[n])
+                print()


make sure you remove unnecessary print statements

Thanks @jeromebarre , I was wondering if I would need to create a test for this. Also, sorry for the slow reply, I was out on vacation last week.

Regarding the test, would it be preferred for me to expand the current test that exists for this IODA converter, or should I add a new, separate test to cover the new functionality?

Regarding the print statements, I had added those just before opening the PR as I thought it may be good for users to have a log of what files were processed, but I've got no problem removing those if that is unneccessary or unwanted.

(edited to add a user tag, as I'm unsure if reply notifications are sent if that isn't done)

BenjaminRuston · 2024-09-19T16:56:20Z

@BlakeJAllen-NOAA are you familiar with how to add a ctest?

here is an example of a new converter (it just added a yaml file to use the bufr2ioda.x executable):
#1539

but you add a small sample input file to test/testinput and a corresponding output to test/testoutput along with the changes to CMakeLists.txt calling the new test and referring to these new files

BlakeJAllen-NOAA · 2024-09-23T20:24:09Z

@BlakeJAllen-NOAA are you familiar with how to add a ctest?

here is an example of a new converter (it just added a yaml file to use the bufr2ioda.x executable): #1539

but you add a small sample input file to test/testinput and a corresponding output to test/testoutput along with the changes to CMakeLists.txt calling the new test and referring to these new files

Thanks, Benjamin, it's good to have an example of adding a ctest. I haven't added any before, but it looks like the process is fairly well documented and plus there is the earlier test of this converter's functionality that I can follow in addition to the info that is in the PR you linked. I'll be busy with other things the next couple of days, but hopefully I will have an chance to work on this later this week.

BenjaminRuston · 2024-10-21T16:38:42Z

@BlakeJAllen-NOAA shall we work on this PR.. this week?

if you have a sample data file, can show the next steps.... Just send an email a short meeting will likely wrap this up.

jeromebarre · 2024-10-22T13:26:20Z

Thanks @BenjaminRuston for the support on this. If need be you can add me on the meeting just as optional, not sure I'll be able to make it.

test/CMakeLists.txt

BenjaminRuston · 2024-10-23T23:15:07Z

@BlakeJAllen-NOAA I'm going to put this into draft mode as well as then you can do many updates and pushes to the repository and not kick off a bunch of regression tests

once we think it's ready, then will remove from draft mode, and run the CI

README.md

fixed typo Co-authored-by: Benjamin Ruston <[email protected]>

BenjaminRuston · 2024-10-29T16:58:07Z

test/CMakeLists.txt

+                          --adp_mask testinput/JRR-ADP_v3r2_j01_s20240110T202044Z_e20240110T202044Z.sample.nc
+                          --adp_conf_lvl 'medhigh'
+                          -o testrun/obs.20240110T202044Z_PT1M_aod.nc
+                          -m nesdis
+                          -k maskout
+                          -n 0.0"


I would advise modifying the code and simplify these, making a few more transparent -n should not have a shorthand and be --thinning_distance (option) and should have a default of 0.0.... the -k should be a logical switch, the adp_conf_level again spelled out a bit more adp_confidence_level and should default to 'medhigh' and then does not need to be present in the call. I can't think of a more verbose name for the --adp_mask but think it too should be more explicit. And the help message should spell out the ADP acronym and have two section describing separately how to run for just AOD, and then how to run for ADP.

At this point OK doing 2 ctests one checking smoke and the other dust; however, this could be done via optional flag requiring two call to the converter

Suggested change

--adp_mask testinput/JRR-ADP_v3r2_j01_s20240110T202044Z_e20240110T202044Z.sample.nc

--adp_conf_lvl 'medhigh'

-o testrun/obs.20240110T202044Z_PT1M_aod.nc

-m nesdis

-k maskout

-n 0.0"

--adp_mask testinput/JRR-ADP_v3r2_j01_s20240110T202044Z_e20240110T202044Z.sample.nc

-o testrun/obs.20240110T202044Z_PT1M_aod.nc

--apply_mask"

BenjaminRuston · 2024-11-19T13:09:34Z

@CoryMartin-NOAA are you using this ?

@BlakeJAllen-NOAA any plans to continue to pursue this?

CoryMartin-NOAA · 2024-11-19T13:46:20Z

@BenjaminRuston this is for the 'simplified' smoke/dust system developed mainly by GSL, so I'm aware but EMC is not actively using it

jeromebarre · 2024-11-19T16:37:42Z

@BlakeJAllen-NOAA, Could you please prioritize finishing this PR? It's important to avoid leaving relatively straightforward PRs like this one unresolved for an extended period. We appreciate your contributions and your responsibility in seeing this through. However, if it's not completed soon, we may need to close it without merging into the develop branch. Thanks!

BlakeJAllen-NOAA and others added 8 commits September 4, 2024 19:55

modified to add identification and separate output of smoke and dust …

6a98a1a

…affected observations

minor changes as needed to pass coding norm test

2966d03

* Change construction of smoke and dust output filenames to work for …

fe2b80f

…more general set of original AOD output filenames

Small changes to satisfy coding norm test

0c339b9

A couple more small changes

9027e15

* Fixed variable name for ADP filenames * Added output of filenames currently being processed * Made --adp_mask filename specification accept multiple filenames (and made it produce a list of strings instead of a string)

BlakeJAllen-NOAA added enhancement New feature or request COMPO Atmospheric COMPOsition labels Sep 11, 2024

BlakeJAllen-NOAA requested review from jeromebarre and bhuang95 September 11, 2024 07:17

BlakeJAllen-NOAA self-assigned this Sep 11, 2024

Merge branch 'develop' into feature/viirs_aod_smoke_dust

7199edf

BlakeJAllen-NOAA requested review from CoryMartin-NOAA and ytangnoaa September 12, 2024 02:35

CoryMartin-NOAA reviewed Sep 12, 2024

View reviewed changes

ytangnoaa reviewed Sep 12, 2024

View reviewed changes

BlakeJAllen-NOAA and others added 2 commits September 12, 2024 17:53

removed unused glob import

5161a8b

Update src/compo/viirs_aod2ioda.py

fa437f7

simplify code for changing case of conf_lvl in function flag_by_confidence_lvl Co-authored-by: Cory Martin <[email protected]>

jeromebarre requested changes Sep 12, 2024

View reviewed changes

jeromebarre reviewed Sep 12, 2024

View reviewed changes

add a stub to show how a ctest should be added

cd9e363

BenjaminRuston reviewed Oct 23, 2024

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

Update test/CMakeLists.txt

7da7124

BenjaminRuston marked this pull request as draft October 23, 2024 23:14

BenjaminRuston and others added 4 commits October 23, 2024 16:16

Merge branch 'develop' into feature/viirs_aod_smoke_dust

be4ee21

initial guess at some sampled files for dust and smoke from AOD ctest

99146f5

add sampled files and what may be a working ctest

537ef23

resolve conflicts

2c082a6

BenjaminRuston reviewed Oct 25, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

084e554

fixed typo Co-authored-by: Benjamin Ruston <[email protected]>

BenjaminRuston reviewed Oct 29, 2024

View reviewed changes

Merge branch 'develop' into feature/viirs_aod_smoke_dust

4fbdba9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/viirs aod smoke dust #1550

Feature/viirs aod smoke dust #1550

BlakeJAllen-NOAA commented Sep 11, 2024

CoryMartin-NOAA Sep 12, 2024

BlakeJAllen-NOAA Sep 12, 2024 •

edited

Loading

ytangnoaa Sep 12, 2024

BlakeJAllen-NOAA Sep 12, 2024

ytangnoaa Sep 12, 2024

BlakeJAllen-NOAA Sep 12, 2024

ytangnoaa Sep 12, 2024

jeromebarre Sep 12, 2024 •

edited

Loading

jeromebarre left a comment

jeromebarre Sep 12, 2024

BlakeJAllen-NOAA Sep 23, 2024 •

edited

Loading

BenjaminRuston commented Sep 19, 2024

BlakeJAllen-NOAA commented Sep 23, 2024

BenjaminRuston commented Oct 21, 2024

jeromebarre commented Oct 22, 2024

BenjaminRuston commented Oct 23, 2024

BenjaminRuston Oct 29, 2024 •

edited

Loading

BenjaminRuston commented Nov 19, 2024

CoryMartin-NOAA commented Nov 19, 2024

jeromebarre commented Nov 19, 2024

Feature/viirs aod smoke dust #1550

Are you sure you want to change the base?

Feature/viirs aod smoke dust #1550

Conversation

BlakeJAllen-NOAA commented Sep 11, 2024

Description

Issue(s) addressed

Dependencies

Impact

Checklist

CoryMartin-NOAA Sep 12, 2024

Choose a reason for hiding this comment

BlakeJAllen-NOAA Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

ytangnoaa Sep 12, 2024

Choose a reason for hiding this comment

BlakeJAllen-NOAA Sep 12, 2024

Choose a reason for hiding this comment

ytangnoaa Sep 12, 2024

Choose a reason for hiding this comment

BlakeJAllen-NOAA Sep 12, 2024

Choose a reason for hiding this comment

ytangnoaa Sep 12, 2024

Choose a reason for hiding this comment

jeromebarre Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

jeromebarre left a comment

Choose a reason for hiding this comment

jeromebarre Sep 12, 2024

Choose a reason for hiding this comment

BlakeJAllen-NOAA Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

BenjaminRuston commented Sep 19, 2024

BlakeJAllen-NOAA commented Sep 23, 2024

BenjaminRuston commented Oct 21, 2024

jeromebarre commented Oct 22, 2024

BenjaminRuston commented Oct 23, 2024

BenjaminRuston Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

BenjaminRuston commented Nov 19, 2024

CoryMartin-NOAA commented Nov 19, 2024

jeromebarre commented Nov 19, 2024

BlakeJAllen-NOAA Sep 12, 2024 •

edited

Loading

jeromebarre Sep 12, 2024 •

edited

Loading

BlakeJAllen-NOAA Sep 23, 2024 •

edited

Loading

BenjaminRuston Oct 29, 2024 •

edited

Loading