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.

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

cmip6_fix_fgoals_f3_l_Amon_time_bnds #831

Merged
merged 7 commits into from
Jan 5, 2021

Conversation

mwjury
Copy link
Contributor

@mwjury mwjury commented Oct 14, 2020

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.

Closes #821

@mwjury mwjury changed the title Add fix and test cmip6_fix_fgoals_f3_l_Amon_time_bnds Oct 14, 2020
@mwjury
Copy link
Contributor Author

mwjury commented Oct 15, 2020

I would tackle the codacy issues. Though there is not much I can do, any ideas.

@mwjury mwjury requested a review from valeriupredoi October 19, 2020 12:29
@mwjury mwjury added the fix for dataset Related to dataset-specific fix files label Oct 22, 2020
Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

couple minor changes and a question, cheers 🍺

for cube in cubes:
if cube.attributes['table_id'] == 'Amon':
time = cube.coord('time')
if cube.attributes['table_id'] == 'Amon' and \
Copy link
Contributor

Choose a reason for hiding this comment

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

you are checking for this already two lines up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

cftime.DatetimeNoLeap(c.year + 1, 1, 1) for c in times
]
time.bounds = time.units.date2num(
np.stack([starts, ends], -1))
Copy link
Contributor

Choose a reason for hiding this comment

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

is the fault discoverable immediately by looking at the first and second [b1, b2] bounds? Or could be somewhere deeper in the bounds array? I also think it's prob best if you just say time.bounds = None; time.guess_bounds() - bit more general?

Copy link
Contributor Author

@mwjury mwjury Nov 2, 2020

Choose a reason for hiding this comment

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

From what I have seen it's concerning all time bounds. I added a check over the whole period though.

Concerning boundary guessing, I don't consider it the way forward, as it simple finds bounds in the middle between the two points, which is not correct in the case of monthly data. Leading to e.g. DimCoord([1850-01-16 12:00:00, 1850-02-15 00:00:00, 1850-03-16 12:00:00 ...] , bounds=[[1850-01-01 18:00:00, 1850-01-31 06:00:00], [1850-01-31 06:00:00, 1850-03-01 18:00:00], [1850-03-01 18:00:00, 1850-03-31 18:00:00], ...]

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - am not a big fan of guessing bounds myself but it may be easier if the result is the same as the coded by hand one, which is not the case here 👍

@mwjury mwjury requested a review from jvegreg as a code owner November 2, 2020 14:05
Co-authored-by: Valeriu Predoi <valeriu.predoi@gmail.com>
@mwjury
Copy link
Contributor Author

mwjury commented Dec 14, 2020

@valeriupredoi same here I assume ;)

@jvegreg jvegreg merged commit dd7b9ea into master Jan 5, 2021
@jvegreg jvegreg deleted the cmip6_fix_fgoals_f3_l_Amon_time_bnds branch January 5, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataset problem: FGOALS-f3-L seasonal statistics fails due to bad time_bnds in raw netcdf
3 participants