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

ENH: Add uarray support for scipy.linalg #114

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Smit-create
Copy link

@Smit-create
Copy link
Author

@rgommers
I have a suggestion for the design:

  1. We can move all the _*_replacer functions to a new file _multimethods_replacers.py and document in each replacer that which functions in _multimethods.py uses it for dispatching since there isn't much to look into these replacer functions for bugs and moreover it reduces the effort to write some common replacer functions like _x_replacer which can be used across multiple submodules.
  2. So, _multimethods.py will purely contain the function definitions which will reduce its size too and easy to maintain.

return a, b, q


def _a_q_replacer(args, kwargs, dispatchables):
Copy link
Owner

Choose a reason for hiding this comment

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

This and _a_b_replacer further down seem identical, as does _A_E_replacer. Why do we need multiple of these, just with a different name to match parameter names? It looks to me like this could be simplified.

Copy link
Author

Choose a reason for hiding this comment

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

Why do we need multiple of these, just with a different name to match parameter names?

Yes, see scipy#14407 (comment)

@rgommers
Copy link
Owner

That will reduce it a little; I suspect we may be able to get rid of some of those replacers completely to make it a small issue. The definitions could also be grouped together at the top of the file, right under _create_linalg = ....

Let's see what @AnirudhDagar and @czgdp1807 think as well.

elif not coerce and not isinstance(value, np.ndarray):
return NotImplemented

return np.asarray(value)
Copy link
Owner

Choose a reason for hiding this comment

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

As we discussed, this:

  • overrides functions which use asanyarray internally, and
  • breaks for sparse matrix inputs

The asanyarray should be looked into per function (and possibly removed, as I think is likely a good idea for clarkson_woodruff), the sparse matrices need handling with an explicit check here and don't need to be coerced.

@czgdp1807
Copy link

The definitions could also be grouped together at the top of the file, right under _create_linalg = ....

Yes. Putting together all the replacers might also help in discovering patterns which we can utilise in future. And yes, some replacers are exactly similar with just different function names.

@Smit-create
Copy link
Author

Linalg looks complete now and ready for higher-level review. Just some refactoring can make it look better. Also, we need to figure out the circular import issue in order to run the whole test suite with scipy backend (i.e. actually using _multimethods functions wherever possible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants