Fix for CMIP6 AWI-ESM-1-1-LR parent time units

ESMValGroup / ESMValCore

ESMValCore: A community tool for pre-processing data from Earth system models in CMIP and running analysis scripts.

https://www.esmvaltool.org

Apache License 2.0

42 stars 38 forks source link

Fix for CMIP6 AWI-ESM-1-1-LR parent time units #2507

Closed brittaGrusdt closed 3 weeks ago

brittaGrusdt commented 1 month ago

Description

Closes #2494

Link to documentation:

Before you get started

[x] ☝ Create an issue to discuss what you are going to do

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.

[x] 🛠 This pull request has a descriptive title and labels
[x] 🛠 Code is written according to the code quality guidelines
[x] 🛠 Unit tests have been added
[x] 🛠 Changes are backward compatible
[x] 🛠 All checks below this pull request were successful

To help with the number pull requests:

🙏 We kindly ask you to review two other open pull requests in this repository

brittaGrusdt commented 1 month ago

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

brittaGrusdt commented 1 month ago

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..

valeriupredoi commented 3 weeks ago

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)

valeriupredoi commented 3 weeks ago

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

schlunma commented 3 weeks ago

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 commented 3 weeks ago

That is correct, Manu! We contacted Codacy but we never got a proper resolution on it, sadly https://github.com/codacy/codacy-coverage-reporter/issues/500

schlunma commented 3 weeks ago

Thanks @brittaGrusdt and @valeriupredoi 🚀

valeriupredoi commented 3 weeks ago

cheers, Manu :beer:

brittaGrusdt commented 2 weeks ago

Thanks a lot, both of you!