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

Jump to bottom

binomial in sage.arith.misc fails for p-adic numbers with negative valuation #35811

Closed
2 tasks done
knospe opened this issue Jun 22, 2023 · 0 comments · Fixed by #35823
Closed
2 tasks done

binomial in sage.arith.misc fails for p-adic numbers with negative valuation #35811

knospe opened this issue Jun 22, 2023 · 0 comments · Fixed by #35823
Labels
c: padics t: bug
Milestone
sage-10.3

Comments

@knospe
Copy link
Contributor

knospe commented Jun 22, 2023

Is there an existing issue for this?

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.

Did you read the documentation and troubleshoot guide?

  • I have read the documentation and troubleshoot guide

Environment

- **OS**: macOS 13.4
- **Sage Version**: 10.1.beta4

Steps To Reproduce

from sage.arith.misc import binomial
K=Qp(3)
binomial(K(1/3),4)

...
ValueError: negative valuation

Expected Behavior

from sage.arith.misc import binomial
K=Qp(3)
binomial(K(1/3),4)

2*3^-5 + 2*3^-4 + 3^-3 + 2*3^-2 + 2*3^-1 + 2 + 2*3 + 2*3^2 + 2*3^3 + 2*3^4 + 2*3^5 + 2*3^6 + 2*3^7 + 2*3^8 + 2*3^9 + 2*3^10 + 2*3^11 + 2*3^12 + 2*3^13 + 2*3^14 + O(3^15)

Actual Behavior

A value error is raised (see above). The problem is that the misc.py only treats a TypeError after trying x=ZZ(x), and not the ValueError that is raised in this case.

For p-adic integers with non-negative valuation, the error does not occur since the the conversion to ZZ actually works. However, the computation might be more complicated than necessary in some cases, since the corresponding integer can be large.
Example: For K=Qp(3), ZZ(K(-1)) gives 3486784400 . But this is less critical.

Additional Information

I have a fix for misc.py with some additional TESTS. The main modification is pretty simple:

# case 2: conversion to integers
    try:
        x = ZZ(x)
    except (TypeError, ValueError):
        pass

Then case 4 (the naive method) will apply in the above situation. Pari (case 3) would sometimes also work, but only for Qp and not for extension fields such as Qq(9) (unsupported coercion from pari). So it's better not to extend case 3 to pAdicFields.
@knospe knospe added the t: bug label Jun 22, 2023
@knospe knospe mentioned this issue Jun 24, 2023
Merged
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this issue Dec 21, 2023
sagemathgh-35823: sage.arith.misc: fixes error in binomial for p-ad…
c7dad3f 
…ic numbers with negative valuation

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
As discussed in sagemath#35811 : in case 2 of `binomial` the fix replaces
`except TypeError` by `except (TypeError, ValueError)`.

 Furthermore, I added examples and tests for binomials of p-adic
numbers.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
Fixes sagemath#35811.
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35823
Reported by: Heiko Knospe
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Dec 22, 2023
sagemathgh-35823: sage.arith.misc: fixes error in binomial for p-ad…
be616c7 
…ic numbers with negative valuation

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
As discussed in sagemath#35811 : in case 2 of `binomial` the fix replaces
`except TypeError` by `except (TypeError, ValueError)`.

 Furthermore, I added examples and tests for binomials of p-adic
numbers.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
Fixes sagemath#35811.
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35823
Reported by: Heiko Knospe
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Dec 23, 2023
sagemathgh-35823: sage.arith.misc: fixes error in binomial for p-ad…
7e09d31 
…ic numbers with negative valuation

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
As discussed in sagemath#35811 : in case 2 of `binomial` the fix replaces
`except TypeError` by `except (TypeError, ValueError)`.

 Furthermore, I added examples and tests for binomials of p-adic
numbers.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
Fixes sagemath#35811.
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35823
Reported by: Heiko Knospe
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this issue Dec 24, 2023
sagemathgh-35823: sage.arith.misc: fixes error in binomial for p-ad…
957da8a 
…ic numbers with negative valuation

    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes sagemath#12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
As discussed in sagemath#35811 : in case 2 of `binomial` the fix replaces
`except TypeError` by `except (TypeError, ValueError)`.

 Furthermore, I added examples and tests for binomials of p-adic
numbers.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
Fixes sagemath#35811.
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#35823
Reported by: Heiko Knospe
Reviewer(s): Travis Scrimshaw
@vbraun vbraun closed this as completed in f33213d Dec 26, 2023
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: padics t: bug
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging a pull request may close this issue.

sage.arith.misc: fixes error in binomial for p-adic numbers with negative valuation knospe/sage
2 participants
@knospe @mkoeppe