-
Notifications
You must be signed in to change notification settings - Fork 60
Fix numpy annotations for pybind v3.0.0 #263
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
base: master
Are you sure you want to change the base?
Conversation
6afe40e to
6c42ca3
Compare
8e8a70c to
8396e1e
Compare
… not throw errors anymore
f0a4df0 to
63f0a13
Compare
63f0a13 to
06f204f
Compare
|
I would be interested in merging this into my fork here. |
|
Given the massive amount of work that this has already required, I would rather merge this here. @sizmailov would you mind having a look when you find the time? Thank you! |
|
I tried reviewing your changes but the addition of pybind11 v3 stubs made it difficult to see what had changed. I added pybind11 v3 tests to my fork before realising you had also implemented them here. |
Nested type (such as in Unions) were not being processed. This is a better implementation of #18 from sizmailov#263
Nested type (such as in Unions) were not being processed. This is a better implementation of #18 from sizmailov#263
|
This is a great effort! Here are some comments: Problem 1: Deeply Nested Conditional Logic in @classmethod
def _from_new_style(cls, resolved_type: ResolvedType) -> Optional[_NumpyArrayAnnotation]:
if resolved_type.parameters is None or len(resolved_type.parameters) == 0:
return None
# Handle nested Annotated: Annotated[Annotated[NDArray[...]], "[m, n]"]
if (
len(resolved_type.parameters) > 1
and isinstance(resolved_type.parameters[0], ResolvedType)
and resolved_type.parameters[0].name in cls.__typing_annotated_names
):
# Nested logic...
else:
# More logic...
# Then multiple elif branches for different array types...
if array_type.name == QualifiedName.from_str("numpy.typing.ArrayLike"):
# ArrayLike specific logic
elif array_type.name == QualifiedName.from_str("numpy.typing.NDArray"):
# NDArray specific logic
elif array_type.name == QualifiedName.from_str("numpy.ndarray"):
# ndarray specific logic
else:
return NoneMaybe you can break this is into smaller, focused methods? @classmethod
def _from_new_style(cls, resolved_type: ResolvedType) -> Optional[_NumpyArrayAnnotation]:
if not cls._validate_new_style_input(resolved_type):
return None
array_type_param, other_params = cls._extract_annotation_params(resolved_type)
if not cls._validate_array_type(array_type_param):
return None
return cls._build_from_array_type(array_type_param, other_params)
@classmethod
def _extract_annotation_params(cls, resolved_type: ResolvedType):
"""Extract array type and other parameters, handling nested annotations."""
# Handle nested annotations separately
@classmethod
def _build_from_array_type(cls, array_type_param, other_params):
"""Build annotation based on specific array type."""
# Delegate to type-specific handlersProblem 2: Inconsistent Error Handling Patterns The code has multiple locations returning None without logging or context. Maybe you could return a @dataclass
class AnnotationParseResult:
annotation: Optional[_NumpyArrayAnnotation]
error: Optional[str] = None
@classmethod
def _from_new_style(cls, resolved_type: ResolvedType) -> AnnotationParseResult:
if resolved_type.parameters is None:
return AnnotationParseResult(None, "No parameters in Annotated type")Problem 3: Hard-coded Dimension Pattern
Problem 4: Loss of Context in
Problem 5: Large PR
|
|
Hi @dyollb, are those all blockers? It would be nice to have this so we can package pycolmap for NixOS. :) @sizmailov ? |
I don't think so but I am not a maintainer. We also need this for |
int->typing.SupportsIntand similarly for floatset/list/dictand generic types intyping->collections.abcequivalentFixNumpyArrayDimTypeVarandFixNumpyArrayDimAnnotationto handle both old-style (pre-v3.0.0) and new-style annotations.np.ndarray[PRIMITIVE_TYPE[*DIMS], *FLAGS]typing.Annotated[numpy.typing.ArrayLike, PRIMITIVE_TYPE, "[*DIMS]", "*FLAGS"]typing.Annotated[numpy.typing.NDArray[PRIMITIVE_TYPE], "[*DIMS]", "*FLAGS"]typing.Annotated[numpy.typing.ndarray[Any, PRIMITIVE_TYPE], "[*DIMS]", "*FLAGS"]typing.Annotated[typing.Annotated[NDArray[...]], "[m, n]", "*FLAGS"]numpy.booldtype since v3.0.0 apparently uses it (instead of the nativebool)parse_annotation_strnot being called on the first nested level of a union, thereby breaking the parsing ofUnion[Annotated[numpy...], None]--numpy-array-wrap-with-annotatedsince v3.0.0 does not anymore throw errors on missing import for dynamic shape variables.Unrelated to v3.0.0 but necessary to run tests locally: ensure the tests are built with the correct C++ standard
These fixes have been working well for us at https://github.com/colmap/colmap
Fixes #264