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 CMIP6 AWI-ESM-1-1-LR parent time units #2507

Conversation

brittaGrusdt
Copy link
Contributor

@brittaGrusdt brittaGrusdt commented Aug 9, 2024

Description

Closes #2494

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:

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 many thanks, Britta! Here wre a couple points from me:

  • first off, no worries about the failed Codacy test, that fails when test artifacts can not be uploaded to Codacy's server due to credential issues since the PR is from a private fork (we tried convince them this is an actual bug at their end, but they seem to want to brush it under the carpet)
  • as for the implementation itself, I'd do it this way: if parent_units in cube.attributes and cube.attributes[parent_units] == bad_value: and remove the check on KeyError below - this is cleaner in that it checks for this particular attribute first, then raises an error that gets quashed if there are any other generic attributes-related issues

Cheers 🍺

@valeriupredoi valeriupredoi added bug Something isn't working enhancement New feature or request fix for dataset Related to dataset-specific fix files labels Aug 12, 2024
@brittaGrusdt
Copy link
Contributor Author

Hi again! Thank you very much for the explanations and suggestions, I appreciate it very much! I updated the code and pushed it again.

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.

looking good, many thanks, Britta! Could you also please clean up the PR decsription ie tick the boxes that are relevant to the PR, and either remove of strikethrough the ones that don't apply here, please? Many thanks! 🍺

esmvalcore/cmor/_fixes/cmip6/awi_esm_1_1_lr.py Outdated Show resolved Hide resolved
@brittaGrusdt
Copy link
Contributor Author

Thanks again! I shortened the line, but now I get the following issue from Codacy: continuation line with same indent as next logical line (E125)
I'm not exactly sure how to resolve this without that codacy complains about an overindentation. I'm sorry for the back and forth, this is my first pull request..

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.

let's try this then

esmvalcore/cmor/_fixes/cmip6/awi_esm_1_1_lr.py Outdated Show resolved Hide resolved
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.

last resort 😁

esmvalcore/cmor/_fixes/cmip6/awi_esm_1_1_lr.py Outdated Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor

Hi @brittaGrusdt - my apologies, I was away for a few days, style issues all fixed, so if you could have a look at the PR description (boxes etc), that'd be great, cheers! I am going to approve now, don't worry about the failed tests - nothing to do with your code changes, I'll ping @schlunma for both a quick check over the PR and merge, and for us to have a look at those tests (Manu, it seems we need to look into the new iris MeshXY that we popped in with your PR)

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.

cheers muchly 🍺

@valeriupredoi
Copy link
Contributor

Aah nevermind, Manu! realized the PR is behind main 😁 But if you have a second to have a look and merge this one, that'd be kickin' 🍺

@schlunma schlunma added this to the v2.12.0 milestone Aug 23, 2024
@schlunma schlunma removed bug Something isn't working enhancement New feature or request labels Aug 23, 2024
@schlunma schlunma changed the title Cmip6 fix awi_esm_1_1_lr parent time units Fix for CMIP6 AWI-ESM-1-1-LR parent time units Aug 23, 2024
@schlunma
Copy link
Contributor

I guess the failing test is again related to the fact that the feature branch is in a fork? https://app.circleci.com/pipelines/github/ESMValGroup/ESMValCore/11565/workflows/1aba8376-eba9-4258-a831-c59b71b890b2/jobs/48634

@valeriupredoi
Copy link
Contributor

That is correct, Manu! We contacted Codacy but we never got a proper resolution on it, sadly codacy/codacy-coverage-reporter#500

@schlunma
Copy link
Contributor

Thanks @brittaGrusdt and @valeriupredoi 🚀

@schlunma schlunma merged commit 343368e into ESMValGroup:main Aug 23, 2024
2 of 3 checks passed
@valeriupredoi
Copy link
Contributor

cheers, Manu 🍺

@brittaGrusdt
Copy link
Contributor Author

Thanks a lot, both of you!

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.

fix_metadata for cmip6 awi_esm_1_1_lr fails if "parent_time_units" isn't in attributes dict
3 participants