Skip to content

mpfdtype: Fix routing issues and add tests. - #263

Open
rabbott999 wants to merge 1 commit into
numpy:mainfrom
rabbott999:main
Open

rabbott999 wants to merge 1 commit into
numpy:mainfrom
rabbott999:main

Conversation

@rabbott999

Copy link
Copy Markdown

These are all issues with where different ufuncs are routed, and adds a test for unary ufuncs which would've caught the issue.

The issues are:

  • The implementations of np.square and np.sqrt are swapped (ironically this doesn't show up in the README example since it uses arr**2 + np.sqrt(a))
  • np.arctan calls mpf_tan instead of mpfr_atan
  • np.exp2 calls exp instead of exp2
  • Functions are implemented for np.sin, np.cos, and np.tan, but they are not registered in umath.cpp.

For full disclosure, I found these issues while pointing an LLM at the codebase, but the changes and write-up are my own.

These are all issues with where different ufuncs are routed, and adds
a test for unary ufuncs which would've caught the issue.

The issues are:

- The implementations of np.square and np.sqrt are swapped (ironically
this doesn't show up in the README example since it uses arr**2 + np.sqrt(a))
- np.arctan calls mpf_tan instead of mpfr_atan
- np.exp2 calls exp instead of exp2
- Functions are implemented for np.sin, np.cos, and np.tan, but they
are not registered in umath.cpp.

@ngoldbaum ngoldbaum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like it's been a while since anyone has run CI in this repo and the failure that's getting hit is pre-existing.

This all seems reasonable, although the exact-equality check with floats looks a little suspicious to me.

Ping @seberg it looks like someone is interested in this code!

@rabbott999 if you're interested in helping us update CI on this repo (it's using Python 3.11 which upstream NumPy dropped support for) and making mpfdtype less of a toy for demonstration purposes, let us know.

@seberg seberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, this was mostly a toy to:

  • Show that something like an mpf dtype is possible
  • Show how a parametric dtype with not super-trivial parametrization works.

So I am curious, are you interested in this or just took a random look?

The changes do look fine to the point where we could just put it in, the CI failure is that isnan() is expected to work and doesn't. Should be easy to fix and would be great to, but arguably a different thing.

I find it a bit strange that == pans out and non-precise test would be better... Although, if it passes (and apparently it does), then maybe it is OK until it fails.

This branch has not been deployed

No deployments
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