-
Notifications
You must be signed in to change notification settings - Fork 60
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 perferred_lib #283
Fix perferred_lib #283
Conversation
@@ -39,7 +39,7 @@ | |||
ArrayLike = Union[Union[DYNAMICS_NUMPY_ALIAS.registered_types()], list] | |||
|
|||
|
|||
def _preferred_lib(*args, **kwargs): |
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.
kwargs
are needed?
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.
If we want to remove **kwargs
the doc string for _numpy_multi_dispatch
should be updated to explicitly state that the dispatching is only impacted by the type of args
.
What issue did you run into that made you want to remove this? The only downside then is if for some reason we need to dispatch on one of the kwargs
. At the moment though I guess we don't need this, so we could re-add it later if necessary.
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.
Can you further explain the list recursion problem?
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.
Sorry for confusing you. It was not accurate. I looked into it in more detail. I guess it is an edge case when it occurs when registering list
type to alias.
At DYNAMICS_NUMPY_ALIAS.infer_libs(args[0])
for second argument,
args[0] is interpreted as list because libs
is not None at https://github.com/Qiskit-Extensions/arraylias/blob/275315c2244f4d1c6351642ed7f6ecad0bf7f847/arraylias/alias.py#L343.
if list
type is not registered, libs
is None and libs
is defined at https://github.com/Qiskit-Extensions/arraylias/blob/275315c2244f4d1c6351642ed7f6ecad0bf7f847/arraylias/alias.py#L345 .
This error doesn't occur if we don't register list
as type to alias.
My PR could fix this issue by deleting kwargs
and changing args[1:]
to *args[1:]
.
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.
Ah I understand now. The follow up questions/comments I have are:
- Why do we need to register
list
again? Was it just forto_dense
andto_sparse
? - Should we have a PR just for registering the new libraries
"scipy_sparse"
and"jax_sparse"
(and"list"
if necessary)? Maybe this change to_preferred_lib
, as well as discussions about how to registerlist
can be included in that PR. It might be beneficial to separate the type registering from the update to themodels
module.
@to24toro this is out of date and can be closed, correct? |
Yes. I closed. |
Summary
The current code included list recursively by using list and I fixed the recursive part.
Details and comments