-
Notifications
You must be signed in to change notification settings - Fork 43
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 remaining numpy mypy issues #521
Conversation
tests/unit/acquisition/test_utils.py
Outdated
@@ -41,7 +42,7 @@ | |||
], | |||
) | |||
def test_split_acquisition_function( | |||
f: AcquisitionFunction, x: np.ndarray, split_size: int, expected_batches: int | |||
f: AcquisitionFunction, x: np.ndarray[Any, Any], split_size: int, expected_batches: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so here we explicitly state the type of objects in arrays, similar as with tuple, List etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: the parameters refer to shape and dtype (though shape isn't properly supported at the moment, and we currently don't handle dtypes well enough to use that part). There's a cleaner shorthand in numpy.typing called npt.NDArray
but that doens't work with older versions of numpy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
# pytest uses yield in a funny way, so we use type ignore | ||
@pytest.fixture(name="keras_float") # type: ignore | ||
def _keras_float() -> None: | ||
@pytest.fixture(name="keras_float") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we then move this fixture up to tests/conftest.py
since we have it now in multiple places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 but planning to tidy up the conftests in a separate PR
This fixes the remaining issues exposed by the numpy stubs introduce in 1.21 and adds a workflow to test this.