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

More mul!, addmul!, submul! methods for ZZRingElemOrPtr #1860

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

fingolfin
Copy link
Member

Specifically, generalize some methods from ZZRingElem to ZZRingElemOrPtr,
and add missing variants for addmul! and submul!

Specifically, generalize some methods from ZZRingElem to ZZRingElemOrPtr,
and add missing variants for addmul! and submul!
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 35.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 86.44%. Comparing base (8ce122a) to head (ffb36b7).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/flint/fmpz.jl 35.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1860      +/-   ##
==========================================
- Coverage   86.46%   86.44%   -0.03%     
==========================================
  Files          98       98              
  Lines       36012    36021       +9     
==========================================
- Hits        31139    31137       -2     
- Misses       4873     4884      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fingolfin
Copy link
Member Author

My main criticism of this PR (that I made ;-) ) is that there are no tests for the new functions. Though there are also none for several of the existing methods ... :-/. I can add some when I find the time, or maybe someone else does, or maybe we just merge this for now if someone verifies that the ccalls look sane, and we add the tests later together with additional refactoring... ah well.

In the meantime I wonder what the HeckeCI error means, and if they are a fluke or in some way caused by this PR (but how?)

    ModAlgAss/Lattices/Lattices.jl          |    12           12
    LocalField/neq.jl                       |   252     2    254
    RCF/conductor_sieve.jl                  |    23           23
    FAILURE

The global RNG seed was 0xe1408e0caa2c92fc9de8b8de6ee5e7d6.

Error in testset LocalField/neq.jl:
Test Failed at /home/runner/.julia/packages/Hecke/dACt3/test/LocalField/neq.jl:57
  Expression: iszero(a) || valuation(a) > 20
Error in testset LocalField/neq.jl:
Test Failed at /home/runner/.julia/packages/Hecke/dACt3/test/LocalField/neq.jl:57
  Expression: iszero(a) || valuation(a) > 20
ERROR: LoadError: LoadError: Test run finished with errors
in expression starting at /home/runner/.julia/packages/Hecke/dACt3/test/parallel.jl:328
in expression starting at /home/runner/.julia/packages/Hecke/dACt3/test/runtests.jl:260
ERROR: Package Hecke errored during testing

@lgoettgens
Copy link
Collaborator

The HeckeCI error is thofma/Hecke.jl#1428

@fingolfin
Copy link
Member Author

Then let's merge this and add the tests systematically a bit later

@fingolfin fingolfin merged commit a47d0f6 into master Sep 24, 2024
22 of 25 checks passed
@fingolfin fingolfin deleted the mh/more-fmpz branch September 24, 2024 19:54
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.

2 participants