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

add some missing test for stream grids with callables #4255

Merged
merged 3 commits into from
Dec 20, 2022

Conversation

chrishavlin
Copy link
Contributor

This PR adds a test for calling load_uniform_grid with data specified via a function. It also moves the whole test_callable_grids.py suite to pytest-only.

@chrishavlin chrishavlin added code frontends Things related to specific frontends pytest infrastructure Related to CI, versioning, websites, organizational issues, etc labels Dec 19, 2022
@neutrinoceros
Copy link
Member

It seems that the errors we're seeing on the image tests job are not related to the known-flakiness (usually it's 7 failures or 0, here we got 4). They are probably due to the recent release of numpy 1.24.0, and they also appear to be so subtle the diff tool doesn't produce anything useful. All that to say: it should be safe to bump. @chrishavlin if you're up for it, can you do that ? Otherwise I can take care of it tomorrow.

@neutrinoceros
Copy link
Member

Actually I went ahead and opened the PR on the answer-store repo: yt-project/answer-store#34

@neutrinoceros
Copy link
Member

for reference: blocked by #4257

@neutrinoceros
Copy link
Member

neutrinoceros commented Dec 20, 2022

Since you opened this after commenting #4244 (comment), I suppose it'd be useful to define a type for the expected function signature while we're at it
Something like

# yt/_typing.py
from typing import Callable
import numpy as np

FieldName = str
FieldFunction = Callable[[Grid, FieldName], np.ndarray]

(I'm just not sure what's the best way to define a Grid type here, maybe a typing.Protocol ? or simply just use YTSelectionContainer ?)

@neutrinoceros
Copy link
Member

@chrishavlin seems like re-running tests doesn't help even after the issue was resolved on the main branch. You'll need to either rebase or merge from main

@chrishavlin
Copy link
Contributor Author

You'll need to either rebase or merge from main

Ok! Will do!

it'd be useful to define a type for the expected function signature while we're at it ....

Sure! Also, if it's easier in any way, it's fine if this PR waits until after 4244 or if this gets merged into 4244 instead of main. It seemed like adding the test was a quick standalone thing, but it's also not a critical change.

or simply just use YTSelectionContainer ?

I think this one... if we wanted to go a little more specific, maybe AMRGridPatch ?

@neutrinoceros
Copy link
Member

Sure! Also, if it's easier in any way, it's fine if this PR waits until after 4244 or if this gets merged into 4244 instead of main. It seemed like adding the test was a quick standalone thing, but it's also not a critical change.

I think it's actually easier to just merge this PR as its own thing. The type hint can be added either here or in my PR, whatever you like best.

or simply just use YTSelectionContainer ?
I think this one... if we wanted to go a little more specific, maybe AMRGridPatch ?

Sure, I was mostly throwing ideas there, but I haven't studied this deep enough to have an actual opinion, so whatever sounds best to you is fine by me !

@chrishavlin
Copy link
Contributor Author

Ok, let's keep this PR to just adding the tests then. I'll rebase and push in a moment. And then I'll look into the type hinting -- if it ends up simple then I think it makes sense to add to 4244, if it is more complicated than expected it might be better as a follow up after 4244. In either case I'll continue this thread over if 4244...

@neutrinoceros neutrinoceros merged commit 30c049b into yt-project:main Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code frontends Things related to specific frontends infrastructure Related to CI, versioning, websites, organizational issues, etc pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants