-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
SFMS: Daily FFMC #4081
SFMS: Daily FFMC #4081
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4081 +/- ##
==========================================
+ Coverage 80.35% 80.37% +0.02%
==========================================
Files 309 309
Lines 11828 11845 +17
Branches 537 537
==========================================
+ Hits 9504 9521 +17
Misses 2138 2138
Partials 186 186 ☔ View full report in Codecov by Sentry. |
api/app/sfms/raster_addresser.py
Outdated
""" | ||
assert_all_utc(timestamp) | ||
iso_date = timestamp.date().isoformat() | ||
return f"sfms/uploads/{run_type.value}/{iso_date}/{weather_param.value}{iso_date.replace('-', '')}.tif" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to get RDPS weather model data? get_model_data_key
does that, but it's stored in a different place in our s3 bucket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, same reasoning as: https://github.com/bcgov/wps/pull/4081/files#r1838823535
api/app/sfms/raster_addresser.py
Outdated
iso_date = timestamp.date().isoformat() | ||
return f"sfms/uploads/{run_type.value}/{iso_date}/fine_fuel_moisture_code{iso_date.replace('-', '')}.tif" | ||
|
||
def get_calculated_daily_ffmc(self, timestamp: datetime): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using the generic get_calculated_index_key
for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar reason to: https://github.com/bcgov/wps/pull/4081/files#r1838825805
api/app/sfms/raster_addresser.py
Outdated
@@ -81,6 +99,16 @@ def get_calculated_precip_key(self, datetime_to_calculate_utc: datetime): | |||
calculated_weather_prefix = f"{self.weather_model_prefix}/{datetime_to_calculate_utc.date().isoformat()}/" | |||
return os.path.join(calculated_weather_prefix, compose_computed_precip_rdps_key(datetime_to_calculate_utc)) | |||
|
|||
def get_daily_ffmc(self, timestamp: datetime, run_type: RunTypeEnum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using get_uploaded_index_key
for this part, maybe makes sense to split them out though, whatever you think works best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_uploaded_index_key
is based on an enum that contains only the abbreviated FWI names, whereas SFMS gives us the full FWI name for FFMC, so I opted for a specific function.
api/app/sfms/daily_ffmc_processor.py
Outdated
logging.warning(f"No ffmc objects found for key: {yesterday_ffmc_key}") | ||
return | ||
|
||
temp_forecast_key = self.addresser.get_daily_model_data_key(current_day, RunType.FORECAST, WeatherParameter.TEMP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we want to use weather_model data we need to keep track of a prediction_hour because we get model data for so many different hours, but maybe there's a better way to do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opted to use all the daily SFMS model data since that's where we get the previous FFMC raster. Not sure if that's correct though. I guess that conflicts with our BUI calculations but the seed FFMC raster will either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I could be wrong but I think the intention was to step forward in these calculations with weather model data (RDPS), since the weather data coming from SFMS is the coarsely interpolated forecasts from BC weather stations
@@ -62,14 +62,15 @@ async def test_bui_date_range_processor(mocker: MockerFixture): | |||
get_weather_data_key_spy = mocker.spy(mock_key_addresser, "get_weather_data_keys") | |||
gdal_prefix_keys_spy = mocker.spy(mock_key_addresser, "gdal_prefix_keys") | |||
get_calculated_index_key_spy = mocker.spy(mock_key_addresser, "get_calculated_index_key") | |||
bui_date_range_processor = BUIDateRangeProcessor(TEST_DATETIME, 2, mock_key_addresser) | |||
bui_date_range_processor = DailyFWIProcessor(TEST_DATETIME, 2, mock_key_addresser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind renaming the bui_date_range_processor
variable and this test case (test_bui_date_range_processor
)?
calculate_bui_spy = mocker.spy(date_range_processor, "calculate_bui") | ||
calculate_dmc_spy = mocker.spy(daily_fwi_processor, "calculate_dmc") | ||
calculate_dc_spy = mocker.spy(daily_fwi_processor, "calculate_dc") | ||
calculate_bui_spy = mocker.spy(daily_fwi_processor, "calculate_bui") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency we could test calculate_ffmc
here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
fwi_keys_exist = await s3_client.all_objects_exist(dc_key, dmc_key) | ||
if not fwi_keys_exist: | ||
logging.warning(f"No previous DMC/DC keys found for {previous_fwi_datetime.date().isoformat()}") | ||
break | ||
|
||
temp_key, rh_key, precip_key = self.addresser.gdal_prefix_keys(temp_key, rh_key, precip_key) | ||
temp_key, rh_key, wind_speed_key, precip_key = self.addresser.gdal_prefix_keys(temp_key, rh_key, wind_speed_key, precip_key) | ||
dc_key, dmc_key = self.addresser.gdal_prefix_keys(dc_key, dmc_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ffmc might need to be included here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
Quality Gate passedIssues Measures |
coverage
and addspytest-cov
dev dependencies for running coverage in VSCode locallyCloses #4053
Test Links:
Landing Page
MoreCast
Percentile Calculator
C-Haines
FireBat
FireBat bookmark
Auto Spatial Advisory (ASA)
HFI Calculator