Skip to content

fix a doctest and a statement#4602

Merged
thofma merged 4 commits intooscar-system:masterfrom
ThomasBreuer:TB_z_vs_zeta
Feb 19, 2025
Merged

fix a doctest and a statement#4602
thofma merged 4 commits intooscar-system:masterfrom
ThomasBreuer:TB_z_vs_zeta

Conversation

@ThomasBreuer
Copy link
Copy Markdown
Member

By default, gen(abelian_closure(QQ)[1])(5) is printed as zeta(5). However, a statement and a doctest in a .md file claimed that z(5) is printed.

(This doctest was not executed in Oscar.build_doc(doctest=true) because it was marked as @jldoctest not as jldoctest --it took me some time until I understood the problem.)

This fix was suggested by @thofma in #4585.

By default, `gen(abelian_closure(QQ)[1])(5)` is printed as `zeta(5)`.
However, a statement and a doctest in a `.md` file claimed that `z(5)`
is printed.

(This doctest was not executed in `Oscar.build_doc(doctest=true)`
because it was marked as `@jldoctest` not as `jldoctest`
--it took me some time until I understood the problem.)
@ThomasBreuer ThomasBreuer added documentation Improvements or additions to documentation topic: number theory release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Feb 18, 2025
@ThomasBreuer ThomasBreuer requested a review from thofma February 18, 2025 13:33
Otherwise all examples that are run later look differently.
@ThomasBreuer
Copy link
Copy Markdown
Member Author

This situation is challenging:
You create abelian_closure(QQ), and the elements of the field have a default printing behaviour.

julia> K, z = abelian_closure(QQ);

julia> z(5)
zeta(5)

julia> Oscar.with_unicode() do
       println(z(5))
       end
ζ(5)

Then you change the default behaviour, for example as follows.
(Internally, this calls set_variable!, which sets a field in K.)

julia> ζ = gen(K, "ζ")
Generator of abelian closure of Q

julia> z(5)
ζ(5)

julia> Oscar.with_unicode() do
       println(z(5))
       end
ζ(5)

But there seems to be no way to get the default behaviour back, i.e., that the non-unicode and unicode printing is different.
For example:

julia> set_variable!(K, "zeta");

julia> z(5)
zeta(5)

julia> Oscar.with_unicode() do
       println(z(5))
       end
zeta(5)

@thofma
Copy link
Copy Markdown
Collaborator

thofma commented Feb 18, 2025

(I wish we would not allow changing the printing.)

Anyway, if one overrides the default, then I don't find it too surprising that with_unicode does not give the default anymore. So for me this looks as expected.

@ThomasBreuer
Copy link
Copy Markdown
Member Author

If one looks at the code then one can expect this behaviour, but is it also intended?

  • There is no way to return to the default behaviour.
  • Overriding the default happens in the call gen(K, "ζ"), which looks harmless at first sight.
  • This situation means that certain tests can be run at most once in a session, and that it is important in which order the tests are run.

@thofma
Copy link
Copy Markdown
Collaborator

thofma commented Feb 18, 2025

Yes, I know the pains, see https://github.com/oscar-system/Oscar.jl/pull/2437/files. I had to add an explicit @test set_variable!(K, "z") at the end. But yes, you are right, this still does not restore the default behavior.

I think there was a reason for this to not be a jldoctest. To move forward with this PR here, I think the two options are:

  • make it not a doctest, so that we can merge it quickly,
  • redesign the printing and how to handle defaults.

Since this was initially only about chaning a z to zeta, maybe we can go with the first option?

@ThomasBreuer
Copy link
Copy Markdown
Member Author

O.k., I turn this into a non-doctest.
Let us see who is the next to fall into this trap.

@fingolfin
Copy link
Copy Markdown
Member

I wish we did not have gen(K, "ζ"). That's a really weird method and behavior

Comment thread docs/src/NumberTheory/abelian_closure.md Outdated
@thofma thofma enabled auto-merge (squash) February 19, 2025 08:18
@ThomasBreuer
Copy link
Copy Markdown
Member Author

I wish we did not have gen(K, "ζ"). That's a really weird method and behavior

Yes. It is not clear to the user that there is just one global object behind abelian_closure(QQ).

Concerning the missing functionality to get back to the default behaviour, if we want to keep backwards compatibility then we could add a new function reset_variable!(K). With that, the tests in question would become safe.

@thofma
Copy link
Copy Markdown
Collaborator

thofma commented Feb 19, 2025

It needs "more work", in that currently we cannot unset K.s, so we need a union with nothing etc.

@thofma thofma merged commit 7de1c37 into oscar-system:master Feb 19, 2025
@ThomasBreuer ThomasBreuer deleted the TB_z_vs_zeta branch February 19, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: number theory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants