Skip to content

Fixed issue in CRT_vectors where moduli are not allowed to be coprime. #40007

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

Merged
merged 7 commits into from
May 11, 2025

Conversation

Noel-Roemmele
Copy link
Contributor

@Noel-Roemmele Noel-Roemmele commented Apr 25, 2025

Fixes #39158. Fixed the issue by creating functionality for CRT_basis if the moduli are not coprime. A more in depth explanation on how CRT_basis was expanded can be found in Here.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@Noel-Roemmele
Copy link
Contributor Author

@DaveWitteMorris. I have completed the expansion for CRT_vectors and CRT_basis as we discussed. I do think it still needs amore detailed explanation in CRT_basis as to what an approximate CRT basis is versus a formal CRT basis. I also still need a good test for the new functionality in CRT_basis.

@Noel-Roemmele
Copy link
Contributor Author

Noel-Roemmele commented Apr 25, 2025

@user202729. Sorry to mention you but it seems like an additional bug detailed Here might also be resolved by this PR. I just wanted to ask if you can confirm that this bug is related and solved by this PR? If it is I will add a Fixes thing at the top.

@user202729
Copy link
Contributor

It (probably?) only fix the second bug (moduli must be coprime), not the first, so I wouldn't count.

Copy link

github-actions bot commented Apr 25, 2025

Documentation preview for this PR (built with commit 4c17ea3; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@DaveWitteMorris
Copy link
Member

The code in lines 3695-3701 is an exact copy of the code in lines 3686-3692. You should refactor to eliminate this code duplication. One possibility might be something like:

try:
    M = prod(moduli)
            ...
        if not d.is_one():
            raise ValueError('moduli must be coprime')
        cs.append((v * Mm) % M)
    if require_coprime_moduli:
        return cs
    # also return a boolean flag to report that the moduli are coprime
    return [cs, True]
except ValueError:
    if require_coprime_moduli:
        raise
    e = [1]
        ...

@Noel-Roemmele
Copy link
Contributor Author

@DaveWitteMorris I have applied your suggestions and removed the code duplication.

@DaveWitteMorris
Copy link
Member

Before the construction of the CRT basis for non-coprime moduli (starting at line 3708), I think you need to add a comment something like:

The following construction of a CRT basis for non-coprime moduli
is explained in a comment on :issue:`39158`.

Noel-Roemmele and others added 2 commits May 2, 2025 03:00
@DaveWitteMorris
Copy link
Member

Running doctests locally was successful, so I expect to set to positive review after the documentation typo is fixed.

Co-authored-by: DaveWitteMorris <[email protected]>
@Noel-Roemmele
Copy link
Contributor Author

@DaveWitteMorris. I've accepted the typo change. Thank you for pointing that out! Everything should be good for a positive review now.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 6, 2025
sagemathgh-40007: Fixed issue in CRT_vectors where moduli are not allowed to be coprime.
    
Fixes sagemath#39158. Fixed the issue by creating functionality for `CRT_basis`
if the moduli are not coprime. A more in depth explanation on how
`CRT_basis` was expanded can be found in
[Here](sagemath#39158).

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [X] The title is concise and informative.
- [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.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#40007
Reported by: Noel-Roemmele
Reviewer(s): DaveWitteMorris, Noel-Roemmele
@vbraun vbraun merged commit aca08c0 into sagemath:develop May 11, 2025
19 of 21 checks passed
@Noel-Roemmele Noel-Roemmele deleted the 39158NonCoprimeCRTVector branch May 21, 2025 08:43
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.

CRT_vectors does not handle non-coprime moduli
4 participants