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

fix N (Issue #103) #106

Merged
merged 1 commit into from
Jan 10, 2022
Merged

fix N (Issue #103) #106

merged 1 commit into from
Jan 10, 2022

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Jan 10, 2022

The evaluation is still slow, but this avoids the overflow. There are two things that could be improved:

  • to check the size of the number before the conversion, to avoid using an exception.
  • the default value of d could be taken from mathics.core.number instead of hardcoding it to 15.

@rocky
Copy link
Member

rocky commented Jan 10, 2022

The evaluation is still slow, but this avoids the overflow.

Great!

There are two things that could be improved:

  • to check the size of the number before the conversion, to avoid using an exception.

For this, bitlength() is what you want.

  • the default value of d could be taken from mathics.core.number instead of hardcoding it to 15.

https://github.com/Mathics3/mathics-core/blob/master/mathics/core/number.py#L28-L35 seems to suggest that the default value would be 17, no? @mmatera where does 15 come into this?

See #107 for a suggestion as to how to use. Feel free to commit over or adapt here otherwise.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 10, 2022

The evaluation is still slow, but this avoids the overflow.

Great!

There are two things that could be improved:

  • to check the size of the number before the conversion, to avoid using an exception.

For this, bitlength() is what you want.

  • the default value of d could be taken from mathics.core.number instead of hardcoding it to 15.

https://github.com/Mathics3/mathics-core/blob/master/mathics/core/number.py#L28-L35 seems to suggest that the default value would be 17, no? @mmatera where does 15 come into this?

@rocky: I used 15, in order to reproduce what we get using Sympy, but you are right, WMA uses 17.

See #107 for a suggestion as to how to use. Feel free to commit over or adapt here otherwise.

@mmatera mmatera merged commit 66bd00f into master Jan 10, 2022
mmatera added a commit that referenced this pull request Jan 10, 2022
This reverts commit 66bd00f.
mmatera added a commit that referenced this pull request Jan 10, 2022
@mmatera
Copy link
Contributor Author

mmatera commented Jan 10, 2022

@rocky, trying to merge your branch into this one, I merge by accident this branch into master. I tried to revert, but it seems it does not work. To avoid more noise, I continue this in #109.

@rocky
Copy link
Member

rocky commented Jan 10, 2022

np

@rocky rocky deleted the fix_103 branch January 24, 2022 03:40
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.

None yet

2 participants