-
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
Add support for daily output frequency #9
Changes from all commits
18964ea
a3f6873
1391ba7
625a911
7fbe763
2ba4f64
49035f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,21 +13,32 @@ | |
# TODO: I might not want to allow this in the ocean and ice fields. Lets see | ||
) | ||
|
||
daily_divisors = sorted([1, 30, 360, 365, 3653]) | ||
|
||
allowed_divisors = { | ||
"Omon": monthly_divisors, | ||
"SImon": monthly_divisors, | ||
"Amon": monthly_divisors, | ||
"day": daily_divisors, | ||
} # Add table_ids and allowed divisors as needed | ||
|
||
|
||
def get_timesteps_simple(dates, table_id): | ||
assert ( | ||
"mon" in table_id | ||
) # this needs some more careful treatment for other timefrequencies. | ||
timesteps = [ | ||
months = [ | ||
(int(d[1][0:4]) - int(d[0][0:4])) * 12 + (int(d[1][4:6]) - int(d[0][4:6]) + 1) | ||
for d in dates | ||
] | ||
if "mon" in table_id: | ||
timesteps = months | ||
elif "day" in table_id: | ||
timesteps = [ | ||
m * 30 for m in months | ||
] # This is close enough without worrying about calendars etc. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment above. I guess dealing with calendars is a whole can of worms, but I am thinking it might be worth to put some of these thoughts into a dedicated issue to keep track of it? Again for now this seems fine for me! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, that sounds like a plan |
||
else: | ||
# this needs some more careful treatment for other timefrequencies. | ||
raise NotImplementedError( | ||
"Only daily and monthly time frequencies are currently supported" | ||
) | ||
return timesteps | ||
|
||
|
||
|
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 these dividers also work generally for other daily data (see here: https://github.com/WCRP-CMIP/CMIP6_CVs/blob/master/CMIP6_table_id.json)?
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 honestly have no idea, I didn't realise there were different
day
table_id and can't find any documentation on what they represent. They might be the same but it seems a bit redundant. Happy to include them or not depending on how much you want to protect against potential future issues.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 will try to check this PR out in the next few days, and maybe run some tests with the 'other' daily frequencies.