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

with_dtype wrapper #8

Merged
merged 2 commits into from Mar 4, 2021
Merged

with_dtype wrapper #8

merged 2 commits into from Mar 4, 2021

Conversation

RikVoorhaar
Copy link
Contributor

fixes #7

This seems to work. Only thing I'm not satisfied with is the comparison

A = fn(*args, **kwargs)
if (dtype is not None) and (dtype != standard_type):
    ...

For example if standard_dtype='float64' and dtype=np.float64 this will still trigger, but it shouldn't.
I have three ways around this:

  • Store standard_type not as string but as backend specific dtype object. Then compare standard_typetoA.dtype` instead.
  • Compare standard_type to get_dtype_name(A).
  • Make a cached function for dtype comparisons across backends

@codecov
Copy link

codecov bot commented Feb 27, 2021

Codecov Report

Merging #8 (ef7f96c) into master (6f14268) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage   97.45%   97.65%   +0.20%     
==========================================
  Files           1        2       +1     
  Lines         510      554      +44     
==========================================
+ Hits          497      541      +44     
  Misses         13       13              
Impacted Files Coverage Δ
autoray/autoray.py 97.63% <100.00%> (+0.18%) ⬆️
autoray/__init__.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f14268...ef7f96c. Read the comment docs.

@jcmgray
Copy link
Owner

jcmgray commented Mar 1, 2021

Looks good. As I understand it standard_type is the dtype that the backend function would return by default? Maybe that could be documented or named slightly more descriptively.

In terms of the comparison, I think get_dtype_name would be my suggestion of the way to go - the idea of that function is indeed to have a simple common representation (i.e. just the string) of backend dtypes. I imagine cached comparisons of backend dtypes can be built on that (the torch_get_dtype_name is already cached for speed e.g.).

@RikVoorhaar
Copy link
Contributor Author

I'm now a bit ambivalent about the standard_type argument here. For some libraries the standard dtype is not constant (in Dask, or Torch we can change it globally), so really it should be a function that returns the current default dtype for the backend. It's cleaner to remove it, and deal with a potentially unnecessary call to get_dtype_name.

I also made sure that all these functions work correctly both when dtype is a string and when dtype is the backend dtype. For torch in particular this was a problem, since it doesn't like getting dtypes as strings.

Finally I made eye always work with a second positional argument.

@RikVoorhaar
Copy link
Contributor Author

Failing test is test_linalg_solve[float32-mars]. Probably my changes also affected gen_rand somehow, but it doesn't seem serious. Moreover get_rand is unnecessary except for complex types?
I didn't pay much attention to complex types, I can try to expand the array creation operations to work with complex types as much as possible, if you think this is important.

@jcmgray
Copy link
Owner

jcmgray commented Mar 4, 2021

Hi @RikVoorhaar , sorry to be slow getting to this - busy time. I think this all looks, thanks again for the PR!

  • Agree - I think the get_dtype_name call is cleaner for now.
  • gen_rand yes is really just a hack for the tests
  • complex types are pretty essential for my work but I don't envisage any problems in the standard functions (zeros, ones, eye etc.) - maybe I'll add some explicit complex tests at some point for certainty / if I encounter problems

@jcmgray jcmgray merged commit 8d331bb into jcmgray:master Mar 4, 2021
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.

Random numbers and dtypes
2 participants