Skip to content

Conversation

@valeriupredoi
Copy link
Contributor

@pp-mo related to #139 here is an idea how I'd approach a warning when the Xarray variable has Numpy data instead of a lazy Dask array. The major hurdle is that most (all?) coordinates do come with realized data points, so I tried to be a bit coy and look for just the main variable data - unfortunately, I don't know Xarray enough to be 100% sure that my approach to checking that is bulletproof. I've added some sample Zarr data locally to the repo (tiny), and also tests, so even if you don't want to keep the warning functionality, I reckon you can still keep the Zarr to Iris Cube conversion tests.

Also, feel free to remove the test that runs with the Zarr store off the S3 bucket if you guys can't connect, but I reckon it'd still be useful in an eg Github Action since there it runs fine. I can mark it with a special pytest mark that runs it eg only in a Github Action.

Cheers and have a good weekend! 🍻

@pp-mo pp-mo self-assigned this Aug 9, 2025
@pp-mo
Copy link
Owner

pp-mo commented Aug 11, 2025

Hi @valeriupredoi
Thanks for this!
I haven't actually found time to check it out properly yet, but please see valeriupredoi#1 which should fix the testing.
And #147 is a mirror just to see what the tests will do when enabled here.

@valeriupredoi
Copy link
Contributor Author

take your time, Patrick, cheers! Excellent, many thanks for porting to a CI-runnable PR, shall do that today 🍻

@pp-mo
Copy link
Owner

pp-mo commented Aug 11, 2025

take your time, Patrick, cheers! Excellent, many thanks for porting to a CI-runnable PR, shall do that today 🍻

Getting there, bit more to fix...!

BTW The doctest failure is a known problem, fix coming in #134 (which is nearly there now).
(
FYI It is not respecting the doctest NORMALIZE_WHITESPACE option ...
It looks like pytest does something weird with doctests settings, not as specified,
also --bizarrely-- intermittent.
)

@pp-mo
Copy link
Owner

pp-mo commented Aug 11, 2025

BTW The doctest failure is a known problem, fix coming in #134 ...

In fact so annoying I've now fixed it separately #149

@valeriupredoi
Copy link
Contributor Author

mega! Do you want me to close this though, so we keep working on in #147 perhaps? 🍻

@pp-mo
Copy link
Owner

pp-mo commented Aug 13, 2025

mega! Do you want me to close this though, so we keep working on in #147 perhaps? 🍻

Hi V!
Sorry for generally slow responses --rather busy elsewhere this week.

#147 was only to check how it would work, and I was intending valeriupredoi#1 to merge my suggestions here.
But I got that wrong by targeting your 'main' !
valeriupredoi#2 fixes, I hope.
Then I think we should continue here, which is under your control, : the problem with #147 is, you can't push to it!

@valeriupredoi
Copy link
Contributor Author

Not a worry @pp-mo - you could add me as a collaborator here to the repo then I can push to branches? 🍺

Copy link
Owner

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Shame you can't edit an existing review ??

Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk>
@pp-mo
Copy link
Owner

pp-mo commented Aug 13, 2025

"and finally..."

Do you want to add a towncrier changenote fragment ? I guess it would be a "dev" one.
Happy to do this for you if you prefer.

I'm not sure if you can just reference the PR, or have to create a suitable issue. I haven't tried it.
I think #139 is not suitable as reference for the changes.

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Aug 13, 2025

@valeriupredoi Sorry just there are a few more suggestions coming -- I found it doesn't pass pre-commit QA checks

Will work that out + suggest those fixes shortly.

let me run it through pre-commit myself, will fix those snags 👍

@valeriupredoi
Copy link
Contributor Author

@valeriupredoi Sorry just there are a few more suggestions coming -- I found it doesn't pass pre-commit QA checks
Will work that out + suggest those fixes shortly.

let me run it through pre-commit myself, will fix those snags 👍

ran pre-commit (with fairly exhaustive black rules) in f8dd7be

@pp-mo
Copy link
Owner

pp-mo commented Aug 14, 2025

Thanks for your patience @valeriupredoi sooo nearly there !

  • decide whether to remove 'unify_chunks'
  • changenote (or ask me to?)

@valeriupredoi
Copy link
Contributor Author

On my way to the office - will remove unify chunks as soon as I get there. Sadly no experience with Towncrier here, so if you could do that for me, that'd be fab 😃🍺

@valeriupredoi
Copy link
Contributor Author

On my way to the office - will remove unify chunks as soon as I get there. Sadly no experience with Towncrier here, so if you could do that for me, that'd be fab 😃🍺

it doesn't take me 2 hours to get to the office 😆 I forgot I had a couple meetings, finally did it in e7df68d - no clue why pre0commit is failing, I isort-ed and pre-commit-ed the test module 😕

@valeriupredoi
Copy link
Contributor Author

nevermind, it passed 🥳

@pp-mo
Copy link
Owner

pp-mo commented Aug 14, 2025

nevermind, it passed 🥳

Probably different versions. It's been evolving a lot lately.
I added pre-commit to the repo, so it fixes itself (mostly).

Look at all the noise, though!

@pp-mo
Copy link
Owner

pp-mo commented Aug 14, 2025

I'm going to make a separate PR with all the pre-commit updates.
Then I'll merge this in a simpler form.

Just to keep the history clean.

@pp-mo
Copy link
Owner

pp-mo commented Aug 14, 2025

#152 replaces : I just removed the pre-commit changes

@pp-mo pp-mo closed this Aug 14, 2025
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants