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

Add support for CNRFC Deterministic Stage Forecasts #76

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

Add support for CNRFC Deterministic Stage Forecasts #76

wants to merge 2 commits into from

Conversation

CAbrahamMBK
Copy link
Contributor

@CAbrahamMBK CAbrahamMBK commented Jan 13, 2023 •
edited
Loading

Closes #75
For the current deterministic stage forecast:

cnrfc.get_deterministic_forecast('SACC1') # only stage data for this point
cnrfc.get_deterministic_forecast('MFPC1') # only flow data for this point
cnrfc.get_deterministic_forecast('NFDC1') # stage and flow data

For the watershed deterministic forecast with stage data:

cnrfc.get_deterministic_forecast_watershed_stage('FeatherYuba', '2023112712')

@CAbrahamMBK CAbrahamMBK requested a review from narlesky as a code owner January 13, 2023 23:59
@CAbrahamMBK CAbrahamMBK added the enhancement New feature or request label Jan 13, 2023
narlesky
narlesky reviewed Jan 14, 2023
View reviewed changes
collect/cnrfc/cnrfc.py Outdated
@@ -150,7 +150,7 @@ def get_water_year_trend_tabular(cnrfc_id, water_year):
'downloaded': dt.datetime.now().strftime('%Y-%m-%d %H:%M')}}


def get_deterministic_forecast(cnrfc_id, truncate_historical=False, release=False):
def get_deterministic_forecast(cnrfc_id, truncate_historical=False, release=False, stage=False):
Copy link
Member

Choose a reason for hiding this comment

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

For this, could we just always return both flow and stage? Would definitely make sense to change the units either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would you do the split between the forecast and historical? this way seems easiest to me for backwards compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay I added support so it will always return flow and stage (if they exist) 9237178
it does the split on the forecast/historical based on the first existing variable (checking for flow first)

narlesky
narlesky reviewed Jan 14, 2023
View reviewed changes
collect/cnrfc/cnrfc.py Outdated
Comment on lines 292 to 309
if keep_stage:
# parse forecast data from CSV
df = pd.read_csv(csvdata,
header=[0, 1],
parse_dates=True,
index_col=0,
float_precision='high',
dtype={'GMT': str})

else:
# parse forecast data from CSV
df = pd.read_csv(csvdata,
header=0,
skiprows=[1,],
parse_dates=True,
index_col=0,
float_precision='high',
dtype={'GMT': str})
Copy link
Member

Choose a reason for hiding this comment

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

Could you refactor for both options so that we use header=[0, 1], ? Might change things downstream a little, but this is probably what we * should * have been doing all along, and is less modification when we want to send the result over to S3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored but preserved the normal single row column header as the default de8208f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

70e87ba renamed the argument to multiindex

narlesky
narlesky reviewed Jan 14, 2023
View reviewed changes
collect/cnrfc/cnrfc.py Outdated
Comment on lines 787 to 789
if keep_stage:
filter_columns = [x for x in df.columns if x[1] != 'SSTG']
df[filter_columns] = df[filter_columns] * 1000.0
Copy link
Member

Choose a reason for hiding this comment

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

Can this filter work for both cases if we keep the multi-level header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this works de8208f

@CAbrahamMBK CAbrahamMBK requested a review from narlesky January 16, 2023 17:44
@narlesky narlesky added feeds support a new data feed needs rebase pretty much ready to merge, but out of date with main labels Feb 8, 2023
@klggill @CAbrahamMBK
get stage data from cnrfc deterministic
39a7590 
get stage data from cnrfc deterministic

add keep_stage option for deterministic forecasts:

fix up deterministic

remove other changes'

remove extra change

remove print statements

fix default string

fix up stage data pull

rename argument to multiindex and add assertion
CAbrahamMBK
CAbrahamMBK commented Nov 28, 2023
View reviewed changes
collect/cnrfc/cnrfc.py
@@ -234,11 +238,11 @@ def get_deterministic_forecast(cnrfc_id, truncate_historical=False, release=Fals
'first_ordinate': first_ordinate.strftime('%Y-%m-%d %H:%M'),
'issue_time': time_issued.strftime('%Y-%m-%d %H:%M'),
'next_issue': next_issue_time.strftime('%Y-%m-%d %H:%M'),
'units': 'cfs',
# 'units': units,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you think we need this?

Copy link
Member

Choose a reason for hiding this comment

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

good q. I think it's important in the case where we convert kcfs to cfs and/or kcfs to acre-feet, but I see how that's a bit sticky here

@CAbrahamMBK CAbrahamMBK added ready for review and removed needs rebase pretty much ready to merge, but out of date with main labels Nov 28, 2023
narlesky
narlesky reviewed Nov 29, 2023
View reviewed changes
collect/cnrfc/cnrfc.py
# convert kcfs/day to cfs/day
df = df * 1000.0
# convert kcfs/day to cfs/day (unless stage data)
filter_columns = [x for x in df.columns if x[1] != 'SSTG']
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a helper function that records the units for the different forecast codes. CNRFC added these helpful notes to https://www.cnrfc.noaa.gov/deterministicHourlyProductCSV.php

Note 1: Data columns labeled QINE and SQIN in the files represent flows in kcfs (thousands of cubic feet per second).
Note 2: Data columns labeled SSTG in the files represent stages in feet.

@narlesky narlesky added the needs rebase pretty much ready to merge, but out of date with main label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narlesky narlesky narlesky left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request feeds support a new data feed needs rebase pretty much ready to merge, but out of date with main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CNRFC stage deterministic forecasts
3 participants
@CAbrahamMBK @narlesky @klggill