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 docstrings for various map construction functions #1431

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Sep 13, 2023

Also include jldoctests for each, and fix a bug in the printing of IdentityMap instances.

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #1431 (7812899) into master (15764fd) will increase coverage by 2.52%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1431      +/-   ##
==========================================
+ Coverage   82.05%   84.58%   +2.52%     
==========================================
  Files         108      109       +1     
  Lines       26777    29363    +2586     
==========================================
+ Hits        21973    24837    +2864     
+ Misses       4804     4526     -278     
Files Changed Coverage Δ
src/Map.jl 100.00% <ø> (ø)
src/generic/Map.jl 68.46% <100.00%> (+26.95%) ⬆️

... and 82 files with indirect coverage changes

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

@fingolfin
Copy link
Member Author

Doctests still fail so I probably made another mistake. I'll work on it later

@thofma
Copy link
Member

thofma commented Sep 13, 2023

Documenter is again maximally useless when reporting the error

src/Map.jl Outdated

# Examples
```jldoctest; setup = :(using AbstractAlgebra)
julia> hom = identity_map(ZZ)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've used the advanced debugging technique of inserting println statements into the Documenter code to learn that this line is causing the problems (or at least it is the first one I am hitting right now).

Documenter first runs this code successfully:

            c = IOCapture.capture(rethrow = InterruptException) do
                Core.eval(sandbox, ex)
            end

where ex is equal to the following quoted code:

begin
    $(Expr(:softscope, true))
    f = AbstractAlgebra.identity_map(AbstractAlgebra.ZZ)
end

But then it proceeds to this, which throws an exception because c.value is not defined:

            Core.eval(sandbox, Expr(:global, Expr(:(=), :ans, QuoteNode(c.value))))

But how is that possible? Hrm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to explain, I know that the access to c.value is throwing the exception because I moved it to a separate line; specifically, I inserted this code before the Core.eval and after the call to `IOCapture.capture:

            println("IOCapture.capture complete")
            println("typeof(c) = $(typeof(c))")
            println("c = $c")
            println("c.value = $(c.value)")

and got this output:

IOCapture.capture complete
typeof(c) = NamedTuple{(:value, :output, :error, :backtrace), Tuple{AbstractAlgebra.Generic.IdentityMap{AbstractAlgebra.Integers{BigInt}}, String, Bool, Vector{Ptr{Nothing}}}}
┌ Error: Doctesting failed
│   exception =
│    UndefRefError: access to undefined reference
│    Stacktrace:
│      [1] show(io::IOBuffer, t::NamedTuple{(:value, :output, :error, :backtrace), Tuple{AbstractAlgebra.Generic.IdentityMap{AbstractAlgebra.Integers{BigInt}}, String, Bool, Vector{Ptr{Nothing}}}})
│        @ Base ./namedtuple.jl:164
...

Very, very strange. I'll try and see if I can use something more advanced like Infiltrator to learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(P.S. I found it a bit faster with JULIA_DEBUG=Documenter) but I don't understand how a named tuple can have undefined references.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infilitrator didn't really help because it could not enter after c was created (I guess that's because it looks at all local variables and touching c in any way blows up).

The type for the NamedTuple also explicitly says that the value component has type AbstractAlgebra.Generic.IdentityMap{AbstractAlgebra.Integers{BigInt}}. So there shouldn't be a way for it to be undefined? But still a UndefRefError is thrown, by getfield... Indeed:

            println("IOCapture.capture complete")
            println("typeof(c) = $(typeof(c))")
            println("nfields(c) = $(nfields(c))")
            println("fieldtype(c,1) = $(fieldtype(typeof(c), 1))")
            println("getfield(c,1) = $(getfield(c, 1))")
            println("c = $c")
            println("c.value = $(c.value)")

results in this output:

IOCapture.capture complete
typeof(c) = NamedTuple{(:value, :output, :error, :backtrace), Tuple{AbstractAlgebra.Generic.IdentityMap{AbstractAlgebra.Integers{BigInt}}, String, Bool, Vector{Ptr{Nothing}}}}
nfields(c) = 4
fieldtype(c,1) = AbstractAlgebra.Generic.IdentityMap{AbstractAlgebra.Integers{BigInt}}
┌ Error: Doctesting failed
│   exception =
│    UndefRefError: access to undefined reference
...

Also include jldoctests for each, and fix a bug in the
printing of IdentityMap instances.
Comment on lines +67 to +70
julia> R, t = ZZ[:t]
(Univariate polynomial ring in t over integers, t)

julia> f = identity_map(R)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not yet figure out what causes the weirdness we saw before, but @thofma found a workaround: instead of ZZ, we use a polynomial ring, then it works.. No idea why, but let's move on for now.

@thofma thofma merged commit 02884f3 into Nemocas:master Sep 13, 2023
15 checks passed
@fingolfin fingolfin deleted the mh/map_docs branch September 13, 2023 14:52
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants