Skip to content

Conversation

@wbhart
Copy link
Contributor

@wbhart wbhart commented Jul 16, 2021

Fixes #556

Also see Nemocas/Nemo.jl#862

This PR is the AbstractAlgebra counterpart to Nemocas/Nemo.jl#1118 however I believe the two PR's to be completely independent.

It's technically breaking as it changes the semantics of root, but should not change any current use cases, so it is weakly breaking in that sense.

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #939 (4014c06) into master (39aadab) will decrease coverage by 0.31%.
The diff coverage is 86.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
- Coverage   82.57%   82.25%   -0.32%     
==========================================
  Files          86       86              
  Lines       20058    20275     +217     
==========================================
+ Hits        16562    16678     +116     
- Misses       3496     3597     +101     
Impacted Files Coverage Δ
src/AbstractAlgebra.jl 41.50% <ø> (ø)
src/julia/Integer.jl 82.22% <86.48%> (+2.97%) ⬆️
src/AbsSeries.jl 90.34% <0.00%> (-5.27%) ⬇️
src/RelSeries.jl 90.16% <0.00%> (-4.73%) ⬇️
src/generic/RelSeries.jl 95.02% <0.00%> (-2.45%) ⬇️
src/generic/AbsSeries.jl 94.11% <0.00%> (-2.44%) ⬇️
src/Rings.jl 67.85% <0.00%> (-1.84%) ⬇️
src/generic/SparsePoly.jl 25.85% <0.00%> (-1.19%) ⬇️
src/generic/LaurentSeries.jl 93.29% <0.00%> (-0.20%) ⬇️
src/Matrix.jl 91.54% <0.00%> (-0.07%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39aadab...4014c06. Read the comment docs.

@fieker
Copy link
Contributor

fieker commented Jul 16, 2021 via email

@wbhart
Copy link
Contributor Author

wbhart commented Jul 16, 2021

The new function does exactly the same thing as the old for any conceivable call. There was no check flag previously, and the default here is false, so.....

As for fmpz, that is handled in the Nemo PR linked above. And as you can see from the docstrings, they have identical semantics.

The difference I am talking about is a technical difference with the original semantics which were to compute the truncated integer part of the root. That's no longer part of the definition of the root function (but still happens in practice anyway).

@wbhart
Copy link
Contributor Author

wbhart commented Jul 16, 2021

Note that the Nemo iroot branch is not expected to pass until flint-2.9 is out (not 2.8), hence the failure on the iroot CI.

@thofma
Copy link
Member

thofma commented Jul 29, 2021

Does the corresponding Nemo PR need flint 2.9 or is 2.8 sufficient?

@wbhart
Copy link
Contributor Author

wbhart commented Jul 29, 2021

2.8 only.

@thofma thofma merged commit 5a27205 into Nemocas:master Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement iroot

3 participants