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

Add type NegInf representing negative infinity #1528

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

fingolfin
Copy link
Member

Also expand the functionality of PosInf to match.

I have deliberately not yet tried to make use of this anywhere, e.g. for the degree of a zero polynomial, as that would likely be a breaking change and thus for a separate PR.

Moreover, it seems to me that this functionality actually should perhaps be moved to AbstractAlgebra... But one step after the other.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #1528 (0d042fd) into master (fd27c97) will increase coverage by 0.04%.
Report is 1 commits behind head on master.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #1528      +/-   ##
==========================================
+ Coverage   82.87%   82.92%   +0.04%     
==========================================
  Files          94       94              
  Lines       37141    37162      +21     
==========================================
+ Hits        30782    30817      +35     
+ Misses       6359     6345      -14     
Files Changed Coverage Δ
src/HeckeMiscInteger.jl 16.21% <ø> (+0.10%) ⬆️
src/HeckeMiscInfinity.jl 95.12% <94.59%> (-4.88%) ⬇️
src/arb/arb.jl 88.30% <100.00%> (+0.57%) ⬆️
src/flint/fmpq.jl 85.74% <100.00%> (+0.14%) ⬆️
src/flint/fmpz.jl 91.22% <100.00%> (+0.18%) ⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@thofma
Copy link
Member

thofma commented Aug 24, 2023

Looks good. You added some signbit methods, which are not used. Could one just loop over more elements in one of the tests?

@fingolfin fingolfin force-pushed the mh/infinity branch 2 times, most recently from 09771fd to 7137c97 Compare September 1, 2023 17:55
Also expand the functionality of `PosInf` to match.
@fingolfin
Copy link
Member Author

I removed Base.signbit(x::acb), as there also is no signbit for Complex (though there is a sign method with weird semantics).

Due to the funky official semantics for sign I also removed these (they are actually for an extension of ours which takes an additional argument; but I did not find a definition for them)

Base.sign(::Type{Int}, ::PosInf) = +1
Base.sign(::Type{Int}, ::NegInf) = -1

For the rest I added more tests. I also added isfinite / isinf methods for QQFieldElem (and fixed isinf(::ZZRingElem), oops).

The existing tests for fmpz, fmpq etc. seem to be rather flaky, too. E.g. things like iseven are often tested with a single input (if in doubt, always an input that leads to true as output).

@fingolfin
Copy link
Member Author

@thofma OK now?

@thofma thofma enabled auto-merge (squash) September 8, 2023 09:12
@thofma thofma disabled auto-merge September 8, 2023 09:12
@thofma thofma merged commit 5781497 into Nemocas:master Sep 8, 2023
14 of 15 checks passed
@lgoettgens
Copy link
Collaborator

Moreover, it seems to me that this functionality actually should perhaps be moved to AbstractAlgebra... But one step after the other.

I can try to include this in #1531 in the upcoming days.

@thofma
Copy link
Member

thofma commented Sep 11, 2023

I overlooked one failure in the doctests of Oscar. I mark this here as breaking.

@fingolfin fingolfin deleted the mh/infinity branch September 11, 2023 13:45
@fingolfin
Copy link
Member Author

Ahhhh, the printing changed and one doctests prints PosInf(), dang

lgoettgens added a commit to oscar-system/Oscar.jl that referenced this pull request Sep 13, 2023
lgoettgens added a commit to oscar-system/Oscar.jl that referenced this pull request Sep 14, 2023
thofma pushed a commit to oscar-system/Oscar.jl that referenced this pull request Sep 14, 2023
* Bump compats

* Remove functions moved to Nemo (Nemocas/Nemo.jl#1519)

* Fix `ZZ` printing (Nemocas/Nemo.jl#1506)

* Fix `identity_map` docstrings (Nemocas/AbstractAlgebra.jl#1431)

* Fix `PosInf` docstring (Nemocas/Nemo.jl#1528)

* Adapt doctests to new printing for maps Nemocas/AbstractAlgebra.jl#1424
fieker pushed a commit to oscar-system/Oscar.jl that referenced this pull request Sep 29, 2023
* Bump compats

* Remove functions moved to Nemo (Nemocas/Nemo.jl#1519)

* Fix `ZZ` printing (Nemocas/Nemo.jl#1506)

* Fix `identity_map` docstrings (Nemocas/AbstractAlgebra.jl#1431)

* Fix `PosInf` docstring (Nemocas/Nemo.jl#1528)

* Adapt doctests to new printing for maps Nemocas/AbstractAlgebra.jl#1424
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.

3 participants