For GUNW workflow with bucket argument, does nothing if SLCs are not within Weather Model's Valid Range #529
Conversation
Add back white space
| HRRR_CONUS_COVERAGE_POLYGON = Polygon(((-125, 21), (-133, 49), (-60, 49), (-72, 21))) | ||
| HRRR_AK_COVERAGE_POLYGON = Polygon(((195, 40), (157, 55), (175, 70), (260, 77), (232, 52))) | ||
| HRRR_AK_PROJ = CRS.from_string('+proj=stere +ellps=sphere +a=6371229.0 +b=6371229.0 +lat_0=90 +lon_0=225.0 ' | ||
| '+x_0=0.0 +y_0=0.0 +lat_ts=60.0 +no_defs +type=crs') | ||
| # Source: https://eric.clst.org/tech/usgeojson/ | ||
| AK_GEO = gpd.read_file(Path(__file__).parent / 'data' / 'alaska.geojson.zip').geometry.unary_union | ||
|
|
There was a problem hiding this comment.
I wanted to use these constants without instantiating classes.
Unfortunately, the HRRR_AK geometry/proj were not reprojecting via geopandas, so I just used a alaska geojson (now included) which is less than 50 KBs.
| from .era5 import ERA5 | ||
| from .era5t import ERA5T | ||
| from .gmao import GMAO | ||
| from .hres import HRES | ||
| from .hrrr import HRRR, HRRRAK | ||
|
|
||
| __all__ = ['HRRR', 'HRRRAK', 'GMAO', 'ERA5', 'ERA5T', 'HRES'] |
There was a problem hiding this comment.
Allows me to import these classes from models instead of having to use each weather model file directly.
There was a problem hiding this comment.
Can you combine this with models.allowed.ALLOWED_MODELS?
There was a problem hiding this comment.
I thought we made it agnostic for users if its HRRR it can do conus and alaska. Adding it under allowed models might break assumptions from @jlmaurer
There was a problem hiding this comment.
Can you combine this with models.allowed.ALLOWED_MODELS?
the __all__ reserved keyword using * with an import as in from RAiDER.models import * (see here). I updated the __init__.py because I wanted to usefrom RAiDER.models import HRES rather than from RAiDER.models.hres import HRES. This is not changing any functionality.
That said, I believe it is good practice to decouple the constant ALLOWED_MODELS and those objects listed in __all__ (partly because flake8 says so: https://stackoverflow.com/a/49266468). I don't think we want __all__ = ALLOWED_MODELS + .... Also, I am including HRRRAK and we could potentially add more constants or functions not all of which are models, so the reverse ALLOWED_MODELS = RAiDER.models.__all__ would not work.
I thought we made it agnostic for users if its HRRR it can do conus and alaska. Adding it under allowed models might break assumptions from @jlmaurer
From what I can see, there is an instance method for _fetch and this does what you are discussing, i.e. fetching the weather model files for CONUS or AK depending on the bounds. I am using properties of the weather model instances and the instantiation does not interact with _fetch (nor should it). Also, fetching will not impact the instance properties - so you have to know which model you are using a priori outside of _fetch. I think designing classes to be initialized based on AOI or something of the like or figuring out if there is an HRRR base class that can handle all this as you say would be at a minimum a different PR if not a larger conversation. Or maybe Jeremy has a suggestion that I am missing here.
| @@ -44,4 +44,5 @@ | |||
| setup( | |||
| ext_modules=cython_extensions + pybind_extensions, | |||
| cmdclass={"build_ext": build_ext}, | |||
| package_data={'tools': ['RAiDER/models/*.zip']} | |||
There was a problem hiding this comment.
@jhkennedy - can you confirm this is done correctly?
I am using https://docs.python.org/3.11/distutils/setupscript.html#installing-package-data
Want to include a zipped file as data.
| # source: https://stackoverflow.com/a/7668273 | ||
| # Allows us to get weather models as strings | ||
| # getattr(module, 'HRRR') will return HRRR class | ||
| module = sys.modules['RAiDER.models'] | ||
| weather_model_names = module.__all__ | ||
| if weather_model_name not in weather_model_names: | ||
| raise ValueError(f'The "weather_model_name" must be in {", ".join(weather_model_names)}') |
There was a problem hiding this comment.
can this be combined with cli.validators.modelName2Module?
There was a problem hiding this comment.
These functions are built in so let's just leave it for now (until another PR). I would have to modify the said function because HRRRAK is in the file HRRR (cli.validators.modelName2Module assumes weather models are in the same file as their name). Let's keep the cli and the GUNW workflow separate for now.
The more I think about this, my opinion is that there should be some constants that can be shared and imported without requiring us to access module attributes or instantiating classes. We should just have a dictionary or variable for valid date range for each model. Initializing an instance of a model is not great particularly because a a given weather model doesn't get initialized based on the bounding box of the AOI so we have to write in control flow anyways based on constants regardless. I think this should be a small separate PR and we can ask Jeremy what he thinks.
|
Given that there is a small issue (maybe it's entirely on job submission side) in regards to whether raider can process data in over spatial/temporal area that should have weather model available, do not want to merge this yet - going to convert to draft until more issues are sorted out. See #542 |
Description
Resolves #526
(I also remove trailing white space in some of the python files that were edited).
An error (
ValueError) is still thrown if HRRR is selected, but this is not within the coverage area - we can easily "do nothing" if this is desired (say we want to process a track across a boundary of HRRR spatial availability).Motivation and Context
Complete workflow for GUNWs if SLC acquisitions do not fall into the weather model's valid date range.
Notes
Modified
hrrr.pyin models to allow for the importing of constants (specifically the coverage areas) without instantiating the weather model classes.