-
Notifications
You must be signed in to change notification settings - Fork 4
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
rewriting gts metrics to pull from ERDDAP #102
Conversation
This also means we no longer need to have this script and we don't need to run that script in this action
|
I have some bigger changes coming in a minute. Switching this to draft for now. |
This should now close #20 I added two new functions.
These two additions are replacements for the notebook https://github.com/ioos/ioos_metrics/blob/main/notebooks/GTS_Totals_weather_act.ipynb. The way I've coded it is very clunky ATM. I'm making two calls to the IOOS ERDDAP to grab the IOOS regional statistics. I think that can be consolidated into one call which grabs all the GTS data, then we filter for what is needed for each plotting routine. I am also getting a bunch of I am open to any suggestions on how to make this code more readable/efficient. |
updating action to no longer ref unnecessary files
The only one that can lead to wrong result is |
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 added comments as to where I think SettingWithCopyWarning
comes in.
@@ -30,7 +34,7 @@ def write_templates(configs, org_config): | |||
|
|||
|
|||
def timeseries_plot(output): | |||
output["date"] = pd.to_datetime(output["date"]) | |||
output["date"] = pd.to_datetime(output.index.strftime("%Y-%m")) |
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 this is where SettingWithCopyWarning
comes in.
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.
Those are probably fine. Is that date
column new or is it getting overwritten?
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.
date
column is new.
|
||
print(f_out) | ||
key = "{} {}".format(f_out.split("_")[3], f_out.split("_")[4].split(".")[0]) | ||
totals_subset['date'] = totals_subset.index.strftime("%Y-%m") |
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 this is where SettingWithCopyWarning
comes in.
From what I can tell, these changes are returning the exact same metrics as was calculated via the previous process. So, I'd like to get these changes in, then we can adjust to limit the number of warnings. How does that sound? I added some line comments on where I think the issue is occurring. |
Starting to address #35
This update removes the requirement to host the
gts/GTS_regional_totals*.csv
files in this repo. Instead, we are harvesting the data directly from the IOOS ERDDAP.I've spot checked a couple of the quarterly summaries and the numbers are the same. However, I would like another set of eyes on it before it gets merged.
@ocefpaf mind taking a look when you get a chance?
We might need to update the Action for generating the webpage to install a few other packages. See https://github.com/ioos/ioos_metrics/blob/main/.github/workflows/website_create_and_deploy.yml
Mainly:
There are probably other opportunities to make this code more readable. Feel free to suggest any changes.