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

Fix for pr of EC_EARTH #1116

Merged
merged 6 commits into from
Jun 7, 2021
Merged

Fix for pr of EC_EARTH #1116

merged 6 commits into from
Jun 7, 2021

Conversation

remi-kazeroni
Copy link
Contributor

@remi-kazeroni remi-kazeroni commented May 10, 2021

Description

Removes erroneously duplicated points in the time coordinate for the last file (time range 2000-2009).

Closes #1111

Link to documentation:


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@remi-kazeroni remi-kazeroni self-assigned this May 10, 2021
@remi-kazeroni remi-kazeroni added AR6 Necessary changes for IPCC AR6 fix for dataset Related to dataset-specific fix files labels May 10, 2021
@remi-kazeroni remi-kazeroni added this to the v2.3.0 milestone May 10, 2021
except iris.exceptions.CoordinateNotFoundError:
pass

return new_list
Copy link
Contributor

@valeriupredoi valeriupredoi May 11, 2021

Choose a reason for hiding this comment

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

@remi-kazeroni a bit of a style note - it's best to have a try/except clause with as little code to try as possible - just one try condition gets executed anyway, and it also looks more readable: try: single clause on one line / except (not clause): raise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valeriupredoi I have modified the fix to take your suggestion into account. Would that work for you now?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks good, cheers!

@remi-kazeroni remi-kazeroni marked this pull request as ready for review June 2, 2021 15:14
@remi-kazeroni remi-kazeroni requested a review from jvegreg as a code owner June 2, 2021 15:14
@remi-kazeroni
Copy link
Contributor Author

remi-kazeroni commented Jun 2, 2021

I mark this PR as RfR even if the tests are failing. I have tried to cover 3 cases:

  1. Cubes with non-monotonic time coordinate get fixed
  2. Cubes with monotonic time coordinate do not get fixed
  3. Cubes with missing time coordinate do not get fixed.

@remi-kazeroni
Copy link
Contributor Author

Tests are now fixed. @valeriupredoi would you have a moment to review this PR? This is my last IPCC related fix. It would be great if we could have it in the next release but I understand this is short notice 😬

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.

very cool @remi-kazeroni 🍺

except iris.exceptions.CoordinateNotFoundError:
pass

return new_list
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good, cheers!

@valeriupredoi valeriupredoi merged commit 797fcbc into main Jun 7, 2021
@valeriupredoi valeriupredoi deleted the fix_ec_earth_pr branch June 7, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AR6 Necessary changes for IPCC AR6 fix for dataset Related to dataset-specific fix files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataset problem: non monotinic time coordinate for the pr variable of EC-EARTH
2 participants