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

Several minor fixes needed for marine BGC data. #2110

Merged
merged 9 commits into from
May 2, 2024
Merged

Conversation

ledm
Copy link
Contributor

@ledm ledm commented Jun 22, 2023

Here are some fairly straightforward fixes for various CMIP6 models. Several of these models are simply not receiving the "standard" OceanFixGrid fixes that they need. Could we instead just apply that to every model - or is that a bad idea?

PR is a draft as it's a work in progress.

@valeriupredoi valeriupredoi added the fix for dataset Related to dataset-specific fix files label Jun 22, 2023
@valeriupredoi valeriupredoi added this to the v2.10.0 milestone Jun 22, 2023
@valeriupredoi
Copy link
Contributor

cheers @ledm - could you pls fix the flake8 issues (line too long etc) 🍺

@zklaus zklaus modified the milestones: v2.10.0, v2.11.0 Oct 9, 2023
@chrisbillowsMO
Copy link
Contributor

Hi @ledm - following on from the community call earlier, would you have time to complete this PR by the end of next week? If not would you be happy to move it to the next milestone v2.12.0 please?

@valeriupredoi
Copy link
Contributor

@ledm am giving you a hand here, so it's merged this m2.11 rather than postpone it yet again 👍

@valeriupredoi
Copy link
Contributor

OK flake8 issues fixed, so we have green tests now, please proceed with work, and let me know when you'd want a review, or please change the milestone to M2.12 and see about it afterwards - up to you @ledm 🍺

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.38%. Comparing base (9912921) to head (0d88785).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2110   +/-   ##
=======================================
  Coverage   94.38%   94.38%           
=======================================
  Files         246      246           
  Lines       13739    13743    +4     
=======================================
+ Hits        12967    12971    +4     
  Misses        772      772           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ledm ledm marked this pull request as ready for review May 1, 2024 12:20
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.

many thanks @ledm LGTM 🍺 @schlunma would you be able to give this one a lookover when you got a minute, plss bud? We can then merge and make the 2.11 cut ✂️

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Would be great if you can add some (minimal) tests for this, e.g.,

def test_get_sos_fix():
"""Test getting of fix."""
fix = Fix.get_fixes('CMIP6', 'BCC-ESM1', 'Omon', 'sos')
assert fix == [Sos(None), GenericFix(None)]
def test_sos_fix():
"""Test fix for ``sos``."""
assert Sos is OceanFixGrid

@valeriupredoi
Copy link
Contributor

good call, Manu! I shall, imminently 🍺

@valeriupredoi
Copy link
Contributor

plopped tests in b7b3fc8 and b9b286d bud 🍺

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thanks @ledm @valeriupredoi 🚀

@schlunma schlunma merged commit 31be51b into main May 2, 2024
6 checks passed
@schlunma schlunma deleted the dev_mpa_fixes branch May 2, 2024 15:01
@valeriupredoi
Copy link
Contributor

and danke Manu 🍺

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.

None yet

5 participants