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 bad parent_time_units for UKESM1-0-LL #274

Closed
wants to merge 7 commits into from
Closed

Conversation

ledm
Copy link
Contributor

@ledm ledm commented Sep 25, 2019

Added a minor fix for UKESM1 cmip6 model. Literally, I just copied and pasted code from the HadGEM3 fix.

@ledm ledm requested a review from valeriupredoi September 25, 2019 10:31
@valeriupredoi
Copy link
Contributor

this should be solved by #231 so even though I am in favour of double-taking, I reckon we should wait for that PR to be merged and then see if there's any more issues 🍺

@valeriupredoi
Copy link
Contributor

OK proves out we still need these type of fixes as @sloosvel pointed out, so @ledm pls fix the PEP hiccup and then it's good to go 🍺

Added extra line for PEP8 compliance.
@ledm
Copy link
Contributor Author

ledm commented Sep 27, 2019

Not sure about the codacy error, it looks like some of our standard behaviours for fixes are not PEP8 compliant.

@mattiarighi mattiarighi added the fix for dataset Related to dataset-specific fix files label Sep 30, 2019
@bouweandela bouweandela requested a review from jvegreg October 15, 2019 13:43
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Could you please add a unit test?

There have been some changes to how the fixes are setup in the last few months, could you please have a look at some other fixes and align this one? That should also solve the Codacy issues.

@bouweandela
Copy link
Member

bouweandela commented Oct 15, 2019

In fact, it looks like this work was already done by @schlunma last month:

"""Fixes for CMIP6 UKESM1-0-LL."""
from .hadgem3_gc31_ll import AllVars as BaseAllVars
class AllVars(BaseAllVars):
"""Fixes for all vars."""

Can this be closed?

try:
if cube.attributes[parent_units] == bad_value:
cube.attributes[parent_units] = 'days since 1850-01-01'
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except AttributeError:
except KeyError:

AttributeError would be raised if cube.attributes did not exist, you probably mean KeyError, which would be raised if there is no key 'parent_time_units' in cube.attributes. Maybe it would be simpler to just write

if cube.attributes.get(parent_units, '') == bad_value:
    cube.attributes[parent_units] = 'days since 1850-01-01'

without the try except

@bouweandela bouweandela changed the title Development ukesm Fix bad parent_time_units for UKESM1-0-LL Nov 15, 2019
@bouweandela
Copy link
Member

@ledm Can you comment on this? #274 (comment) Is this still needed?

@schlunma
Copy link
Contributor

schlunma commented May 4, 2021

I'm pretty sure this can be closed @ledm?

We got this in the current master:

class AllVars(Fix):
"""Fixes for all vars."""
def fix_metadata(self, cubes):
"""Fix parent time units.
Parameters
----------
cubes : iris.cube.CubeList
Input cubes.
Returns
-------
iris.cube.CubeList
"""
parent_units = 'parent_time_units'
bad_value = 'days since 1850-01-01-00-00-00'
for cube in cubes:
try:
if cube.attributes[parent_units] == bad_value:
cube.attributes[parent_units] = 'days since 1850-01-01'
except AttributeError:
pass
return cubes

@schlunma
Copy link
Contributor

schlunma commented Apr 2, 2024

Closing this (see comment above), feel free to reopen if necessary.

@schlunma schlunma closed this Apr 2, 2024
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.

5 participants