Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sbaf nn #97

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Sbaf nn #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c4ebc2
Fixed bug. 'options' is not an option to argparse
salomoneliassonSMHI Oct 18, 2024
6e4159f
Added NN-based SBAF routines and adapted code
salomoneliassonSMHI Oct 18, 2024
f0f72e1
Merge branch 'main' into SBAF_NN
salomoneliassonSMHI Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions bin/vgac2pps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,9 @@
parser.add_argument('-all_ch', '--all_channels', action='store_true',
help="Save all 21 channels to level1c4pps file.")
parser.add_argument('-n19', '--as_noaa19',
choices=[version for version in SBAF],
default=None,
help=("Save only the AVHRR (1,2, 3B, 4, 5) channels to level1c4pps file. "
"And apply SBAFs to the channels."))
help=(f"Save only the AVHRR (1,2, 3B, 4, 5) channels to level1c4pps file. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f-string should only be used if you want to include variables, so remove the f from this row.

f"And apply SBAFs to the channels. The SBAF options are {[version for version in SBAF]}"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The choices you have are already displayed in the help message by default, so not needed to be listed twice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are they? I think something was added, but it threw an error, so I moved it into this help section

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that the help message below describes the choices you have. To me it just adds noise if you add them also in the description.

$ python bin/vgac2pps.py --help
usage: vgac2pps.py [-h] [-o [OUT_DIR]] [-ne [NC_ENGINE]] [-all_ch] [-n19 {v2,v3,v4,v5,v6}] [-pps_ch] [-avhrr_ch] [-on [ORBIT_NUMBER]] [--don_split_files_at_midnight] fileN [fileN ...]

Script to produce a PPS-level1c file for a VIIRS level-1 scene

positional arguments:
  fileN                 List of VIIRS files to process

optional arguments:
  -h, --help            show this help message and exit
  -o [OUT_DIR], --out_dir [OUT_DIR]
                        Output directory where to store the level1c file
  -ne [NC_ENGINE], --nc_engine [NC_ENGINE]
                        Engine for saving netcdf files netcdf4 or h5netcdf (default).
  -all_ch, --all_channels
                        Save all 21 channels to level1c4pps file.
  -n19 {v2,v3,v4,v5,v6}, --as_noaa19 {v2,v3,v4,v5,v6}
                        Save only the AVHRR (1,2, 3B, 4, 5) channels to level1c4pps file. And apply SBAFs to the channels.
  -pps_ch, --pps_channels
                        Save only the necessary (for PPS) channels to level1c4pps file.
  -avhrr_ch, --avhrr_channels
                        Save only the AVHRR (1,2, 3B, 4, 5) channels to level1c4pps file.
  -on [ORBIT_NUMBER], --orbit_number [ORBIT_NUMBER]
                        Orbit number (default is 00000).
  --don_split_files_at_midnight
                        Don't split files at midnight, keep as one level1c file.

Copy link
Contributor

@BengtRydberg BengtRydberg Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if python bin/vgac2pps.py --help throw an error for you, you should fix it :). I think this is not covered by any of the current test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But where does that help message come from?

Copy link
Contributor

@BengtRydberg BengtRydberg Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the argparse library, so it is not our code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Long story short. I'll remove the SBAF version info from the help argument

parser.add_argument('-pps_ch', '--pps_channels', action='store_true',
help="Save only the necessary (for PPS) channels to level1c4pps file.")
parser.add_argument('-avhrr_ch', '--avhrr_channels', action='store_true',
Expand Down
83 changes: 80 additions & 3 deletions level1c4pps/vgac2pps_lib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import os
import time
from satpy.scene import Scene
from sbafs_ann.convert_vgac import convert_to_vgac_with_nn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this module is missing

Copy link
Collaborator Author

@salomoneliassonSMHI salomoneliassonSMHI Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what to do about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Nina is on vacation today, but I talked to her the last week. She said she plans to add this package to PyPI, but it is not yet there. When it is there we need to add it as a dependency in the setup.py file in the same way as e.g. satpy is listed, but until this is fixed all tests will fail on github even if you can run it locally...

from level1c4pps import (get_encoding, compose_filename,
set_header_and_band_attrs_defaults,
rename_latitude_longitude,
Expand Down Expand Up @@ -66,7 +67,8 @@

MBAND_PPS = ["M05", "M07", "M09", "M10", "M11", "M12", "M14", "M15", "M16"]

MBAND_AVHRR = ["M05", "M07", "M12", "M15", "M16"]
# "M10", "M14" are needed for NN SABAF, but are deleted before saving
Copy link
Contributor

@BengtRydberg BengtRydberg Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments about what happens in other parts of the code should be avoided.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the comment to

"M10", "M14" are not AVHRR channels, but needed for NN SABAF

MBAND_AVHRR = ["M05", "M07", "M12", "M15", "M16", "M10", "M14"]

MBAND_DEFAULT = ["M05", "M07", "M09", "M10", "M11", "M12", "M14", "M15", "M16"]

Expand Down Expand Up @@ -323,9 +325,77 @@
"slope": 1.002,
"offset": -0.69,
"comment": "Based on collocation data for VZA < 3 and SZA 0-180",
},
},
"KNMI": {
"r06": {
"viirs_channel": "M05",
"slope": 0.9401,
"offset": 0.629,
"comment": "VZA<60, SZA<60, Delta(VZA) < 5, Delta(Scat-angle) < 5",
},
"r09": {
"viirs_channel": "M07",
"slope": 0.9345,
"offset": -1.304,
"comment": "VZA<60, SZA<60, Delta(VZA) < 5, Delta(Scat-angle) < 5",
},
"tb37_night": {
"viirs_channel": "M12",
"slope": 0.9934,
"offset": 1.659,
"min_sunzenith": 89,
"comment": " VZA<60, SZA>95, Delta(VZA) < 5",
},
"tb37_day": {
"viirs_channel": "M12",
"slope": 0.9572,
"offset": 9.572,
"max_sunzenith": 80,
"comment": "VZA<60, SZA<60, Delta(VZA) < 5, Delta(Scat-angle) < 5",
},
"tb37_twilight": {
"viirs_channel": "M12",
"slope": 0.9753,
"offset": 5.6155,
"min_sunzenith": 80,
"max_sunzenith": 89,
"comment": "The linear average of the SBAFs for t37_day and t37_night. 80<= SZA <89",
},
"tb11": {
"viirs_channel": "M15",
"slope": 0.9986,
"offset": 0.600,
"comment": "",
},
"tb12": {
"viirs_channel": "M16",
"slope": 0.9943,
"offset": 1.328,
"comment": "",
},
},
"NN_v1": {

"datadir": "/nobackup/smhid20/proj/safcm/data/SBAFS_NN/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we must include the network coefficient file in this package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense.

"comment": "NN based on AVHRR and VGAC matchups using all AVHRR heritage channels"
}
}
}


def convert_to_noaa19_NN(scene, sbaf_version):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no capital letters in function name

"""Applies AVHRR SBAF to VGAC channels using NN approach"""

if sbaf_version == "NN_v1":
sbaf_nn_dir = SBAF[sbaf_version]['datadir']
else:
logger.error(f"Unrecognized NN version, {sbaf_version}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need logger.exception here, that will also raise an exception, otherwise you will end up with an uncontrolled exception later

scene = convert_to_vgac_with_nn(scene, SBAF_NN_DIR=sbaf_nn_dir)

logger.info(f'Created NN version {sbaf_version}')
if "npp" in scene.attrs["platform"].lower():
scene.attrs["platform"] = "vgacsnpp"
scene.attrs["platform"] = scene.attrs["platform"].replace("noaa", "vgac")


def convert_to_noaa19(scene, sbaf_version):
Expand Down Expand Up @@ -502,7 +572,14 @@ def process_one_scene(scene_files, out_path, engine="h5netcdf",
sensor = "viirs"
if noaa19_sbaf_version is not None:
sensor = "avhrr"
convert_to_noaa19(scn_, noaa19_sbaf_version)
if "NN" not in noaa19_sbaf_version:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code will be more easy to test if you add this logic to the convert_to_noaa19 function. So I suggest that from this function you call convert_to_noaa19(scn_, noaa19_sbaf_version) as before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I think I understand and I moved the logic into convert_to_noaa19

convert_to_noaa19(scn_, noaa19_sbaf_version)
# These are read in case SBAF is NN based, but need to be removed
# before saving bbecause they are not AVHRR heritage channels
del scn_["M10"]
del scn_["M14"]
else:
convert_to_noaa19_NN(scn_, noaa19_sbaf_version)

filename = compose_filename(scn_, out_path, instrument=sensor, band=irch)
encoding = get_encoding_viirs(scn_)
Expand Down
Loading