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

Solve bug in ACCESS1 dataset fix for calendar. #671

Merged
merged 6 commits into from
Jun 15, 2020

Conversation

Peter9192
Copy link
Contributor

@Peter9192 Peter9192 commented Jun 8, 2020

The access1 datasets have time units Unit('days since 0001-01-01', calendar='proleptic_gregorian'). The existing fix only changes the calendar, but that results in a wrong interpretation of the time points, where the days are offset by ~2 days. This PR changes the time offset first to 'days since 1850-01-01', using convert_units to make sure that time points are updated as well. For the recent period (after 1582 or so), both calendars should be the same, and hence the calendar can now safely be changed to 'gregorian'.

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
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • 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.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #669

@Peter9192
Copy link
Contributor Author

Peter9192 commented Jun 8, 2020

I'm not sure if the current test actually represents an actual case, or it's just a random different calendar. Also not sure if it's okay to just test the calendar name without seeing if the data are still okay.

I'll try to add a test case for the proleptic_gregorian calendar with different time offset tomorrow.

@Peter9192 Peter9192 marked this pull request as draft June 8, 2020 16:23
@Peter9192
Copy link
Contributor Author

I updated the test cases, now including two reference dates: one representative for e.g. piControl, one for more recent dates. The updated test checks not just the name of the calendar, but also if the represented datetimes are unaffected.

Only question I have is: why did the original test case use a 'julian' calendar? Was that a random pick, or was it based on a real use case? I'm guessing it was random, for all I see on ESGF is proleptic_gregorian. So I'm assuming it's safe to change the test. Is that okay, @jvegasbsc ?

@katjaweigel can you verify that this updated works for your use cases as well?

@jvegreg
Copy link
Contributor

jvegreg commented Jun 9, 2020

So I'm assuming it's safe to change the test. Is that okay, @jvegasbsc ?

Yes, it's ok

@Peter9192 Peter9192 marked this pull request as ready for review June 9, 2020 10:52
@valeriupredoi valeriupredoi changed the title Solve bug in access1 dataset fix for calendar. Solve bug in ACCESS1 dataset fix for calendar. Jun 9, 2020
@valeriupredoi
Copy link
Contributor

It's fine but what puzzles me is why we have not had this fix in from the get going ie has ACCESS1 data changed in the meantime? 🍺

@katjaweigel
Copy link
Contributor

Thanks for the fix, I start a test now.

@katjaweigel
Copy link
Contributor

I get an error with concatenating cubes running esmvaltool/recipes/recipe_deangelis15nat.yml with access1_0 and this fix, testing now if the fix is really the reason or if it is not related.

@Peter9192
Copy link
Contributor Author

Thanks for checking! One side-effect that I noticed is that years before 1850 now get negative time points (like 1 Jan 1849 is "-365 days since 1850-01-01") , but I suppose that shouldn't matter for concatenating the cubes, or does it?

@katjaweigel
Copy link
Contributor

Seems to be cause by the fix, the error does not occur with the current ESMValCore master (I added two prints for the time but the error occurred also without):
time1
DimCoord([2006-01-16 12:00:00, 2006-02-15 00:00:00, 2006-03-16 12:00:00, ...,
2100-10-16 12:00:00, 2100-11-16 00:00:00, 2100-12-16 12:00:00], bounds=[[2006-01-01 00:00:00, 2006-02-01 00:00:00],
[2006-02-01 00:00:00, 2006-03-01 00:00:00],
[2006-03-01 00:00:00, 2006-04-01 00:00:00],
...,
[2100-10-01 00:00:00, 2100-11-01 00:00:00],
[2100-11-01 00:00:00, 2100-12-01 00:00:00],
[2100-12-01 00:00:00, 2101-01-01 00:00:00]], standard_name='time', calendar='proleptic_gregorian', long_name='time', var_name='time')
time2
DimCoord([2006-01-16 12:00:00, 2006-02-15 00:00:00, 2006-03-16 12:00:00, ...,
2100-10-16 12:00:00, 2100-11-16 00:00:00, 2100-12-16 12:00:00], bounds=[[2006-01-01 00:00:00, 2006-02-01 00:00:00],
[2006-02-01 00:00:00, 2006-03-01 00:00:00],
[2006-03-01 00:00:00, 2006-04-01 00:00:00],
...,
[2100-10-01 00:00:00, 2100-11-01 00:00:00],
[2100-11-01 00:00:00, 2100-12-01 00:00:00],
[2100-12-01 00:00:00, 2101-01-01 00:00:00]], standard_name='time', calendar='gregorian', long_name='time', var_name='time')
2020-06-09 15:08:52,511 UTC [18856] WARNING There were warnings in variable rsdt:
rsdt: attribute positive not present

2020-06-09 15:08:52,512 UTC [18856] ERROR Failed to run concatenate([<iris 'Cube' of toa_incoming_shortwave_flux / (W m-2) (time: 1140; latitude: 145; longitude: 192)>], {})

I'll try to find out what happens ...

@valeriupredoi
Copy link
Contributor

@bouweandela and myself have recently fixed a bug in concatenation, please merge the latest master in your working branch; no, negative number of days are absolutely normal for units of days since 1950 and actual dates before 1950 🍺

@Peter9192
Copy link
Contributor Author

So it seems in the first print, the calendar has not been fixed. But I wonder why...

@katjaweigel
Copy link
Contributor

katjaweigel commented Jun 9, 2020

Thanks @valeriupredoi , you are right, if I merge in master into fix_access1_calendar:
git checkout fix_access1_calendar
Switched to branch 'fix_access1_calendar'
Your branch is up to date with 'origin/fix_access1_calendar'.
git merge origin/master
it works! (The branch was rather new, I did not expect a non intended difference to master, sorry.)

@Peter9192

So it seems in the first print, the calendar has not been fixed. But I wonder why...

It should be, I printed the time before and after the fix to see the difference.

@Peter9192
Copy link
Contributor Author

before and after

Ah okay, I misunderstood. I thought they were the 2 cubes that had to be concatenated. Thanks again for testing!

I merged master into this PR, so now everything should work fine. Do you want to test it again?

@Peter9192 Peter9192 requested a review from katjaweigel June 10, 2020 07:10
@katjaweigel
Copy link
Contributor

I guess it should work because but I started it with the update to be really sure, takes about 20 minutes (but usually only 3 if it fails).

@katjaweigel
Copy link
Contributor

Thanks you, after merging the master it works.

@bouweandela bouweandela added bug Something isn't working fix for dataset Related to dataset-specific fix files labels Jun 15, 2020
@bouweandela bouweandela added cmor Related to the CMOR standard and removed cmor Related to the CMOR standard labels Jun 15, 2020
@mattiarighi mattiarighi merged commit 6b3702c into master Jun 15, 2020
@mattiarighi mattiarighi deleted the fix_access1_calendar branch June 15, 2020 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in ACCESS1-0 calender fix that modifies the time point values unintentionally.
6 participants