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

Ensure dummy data for cmorize_obs_woa test are written to the correct directory #2451

Merged
merged 1 commit into from Jan 12, 2022

Conversation

ehogan
Copy link
Contributor

@ehogan ehogan commented Jan 6, 2022

Description


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.

…correct directory; test module now runs in ~18 seconds
@ehogan ehogan self-assigned this Jan 6, 2022
@ehogan ehogan added AutoAssess Issues relevant to the conversion of AutoAssess metrics from Met Office to ESMValTool testing labels Jan 6, 2022
@ehogan ehogan added this to In progress in AutoAssess via automation Jan 6, 2022
@valeriupredoi
Copy link
Contributor

cheers @ehogan - could you please also add #2452 that this will close too? I'd like to get a confirm from @remi-kazeroni first there that the WOA data structure is correct, the variables dirs seem fishy to me (but I am probably wrong) 🐟

@valeriupredoi valeriupredoi added observations and removed AutoAssess Issues relevant to the conversion of AutoAssess metrics from Met Office to ESMValTool labels Jan 6, 2022
AutoAssess automation moved this from In progress to Review in progress Jan 6, 2022
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 for the quick fixes, Emma! I would not do the iris submodule imports here - I know the test won't run in single-test run otherwise, but I'd much more prefer a general fix for all the tests like I did in ESMValGroup/ESMValCore#1367 - in a separate PR, of course πŸ‘

@@ -5,6 +5,7 @@
import sys

import iris
from iris import coord_systems, fileformats
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not do this just here but rather have a general fix like here ESMValGroup/ESMValCore#1367

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? To me, this makes perfect sense. Something is used here, so it is imported here.

Copy link
Contributor

Choose a reason for hiding this comment

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

this #2455 is a slightly more general approach in my view and should secure cases when new tests are written and these imports are omitted

Copy link
Contributor

Choose a reason for hiding this comment

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

That PR certainly is more comprehensive. Still, the imports here look like perfectly idiomatic Python to me, so I see no reason to discourage them.

Copy link
Contributor

Choose a reason for hiding this comment

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

well OK, I am fine now once I've put together #2455 - I was just afraid I'd forget doing that forever if I let this through - more of a self-reminder hijacking poor Emma's work 😁

AutoAssess automation moved this from Review in progress to Reviewer approved Jan 11, 2022
@valeriupredoi
Copy link
Contributor

OK then, cheers muchly @ehogan - good to merge by me, @zklaus you good for a merge too?

@ehogan
Copy link
Contributor Author

ehogan commented Jan 11, 2022

@valeriupredoi I did have a quick question about this; is there a way to switch off the automatic download of data for tests? The user configuration file used in these tests doesn't request that the data are automatically downloaded, so I wondered if it was different for cmorizers and whether there was an option to switch it off? This would prevent issues like this from happening again :)

@valeriupredoi
Copy link
Contributor

@ehogan that's a good idea! The OBS automatic downloads happen through a different mechanism than the CMIP data downloads - something @jvegreg and @remi-kazeroni have worked on feverishly all through the latter part of last year; sadly, I don't know much about how that happens and how to configure it to not do it, Remi, you mind chipping in here please mate?

@valeriupredoi
Copy link
Contributor

@ehogan that's a good idea! The OBS automatic downloads happen through a different mechanism than the CMIP data downloads - something @jvegreg and @remi-kazeroni have worked on feverishly all through the latter part of last year; sadly, I don't know much about how that happens and how to configure it to not do it, Remi, you mind chipping in here please mate?

I am going to merge this now, close the specific related issues, and open a separate issue about not downloading OBS data in tests should any inconsistencies/faults occur - even if we knew how to implement that (I don't at this point) we shouldn't do it in this PR anyways, but rather, in a more general one πŸ‘ Many thanks for the good work @ehogan 🍺

@valeriupredoi valeriupredoi merged commit 481efa2 into main Jan 12, 2022
AutoAssess automation moved this from Reviewer approved to Done Jan 12, 2022
@valeriupredoi valeriupredoi deleted the 2445_fix_cmorize_obs_woa_test branch January 12, 2022 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

WOA cmorizer test very slow since it downloads new version data
3 participants