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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
+4 −5
Closed

Fix perferred_lib #283

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions qiskit_dynamics/arraylias/alias.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
ArrayLike = Union[Union[DYNAMICS_NUMPY_ALIAS.registered_types()], list]


def _preferred_lib(*args, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kwargs are needed?

Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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:].

Copy link
Collaborator

@DanPuzzuoli DanPuzzuoli Nov 2, 2023 •
edited
Loading

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 for to_dense and to_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 register list can be included in that PR. It might be beneficial to separate the type registering from the update to the models module.

def _preferred_lib(*args):
"""Given a list of args and kwargs with potentially mixed array types, determine the appropriate
library to dispatch to.

Expand All @@ -48,18 +48,17 @@ def _preferred_lib(*args, **kwargs):

Args:
*args: Positional arguments.
**kwargs: Keyword arguments.
Returns:
str
Raises:
QiskitError: if none of the rules apply.
"""
args = list(args) + list(kwargs.values())
args = list(args)
if len(args) == 1:
return DYNAMICS_NUMPY_ALIAS.infer_libs(args[0])

lib0 = DYNAMICS_NUMPY_ALIAS.infer_libs(args[0])[0]
lib1 = _preferred_lib(args[1:])[0]
lib1 = _preferred_lib(*args[1:])[0]

if lib0 == "numpy" and lib1 == "numpy":
return "numpy"
Expand All @@ -82,5 +81,5 @@ def _numpy_multi_dispatch(*args, path, **kwargs):
Returns:
Result of evaluating the function at path on the arguments using the preferred library.
"""
lib = _preferred_lib(*args, **kwargs)
lib = _preferred_lib(*args)
return DYNAMICS_NUMPY_ALIAS(like=lib, path=path)(*args, **kwargs)
Loading