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

Pretty printing for elliptic curves and their points #1677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonbrandhorst
Copy link
Collaborator

No description provided.

@thofma
Copy link
Owner

thofma commented Nov 8, 2024

I think the "short/compact" form should not contain line breaks? I vaguely remember something like that

@simonbrandhorst
Copy link
Collaborator Author

You are right. Did I overlook a linebreak somewhere?

print(io, "Elliptic curve with equation\n")
io = pretty(io)
print(io, "Elliptic curve with equation ")
print(io, Indent())
Copy link
Owner

Choose a reason for hiding this comment

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

I think I got confused by this. What does this do without linebreak?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing (useful). I overlooked it. Thanks for pointing it out. Removed it.

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Project coverage is 75.95%. Comparing base (39fc9cb) to head (0aedd33).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/EllCrv/Isomorphisms.jl 0.00% 7 Missing ⚠️
src/EllCrv/EllCrv.jl 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1677      +/-   ##
==========================================
- Coverage   75.97%   75.95%   -0.03%     
==========================================
  Files         361      361              
  Lines      113876   113892      +16     
==========================================
- Hits        86518    86502      -16     
- Misses      27358    27390      +32     
Files with missing lines Coverage Δ
src/EllCrv/Finite.jl 93.93% <ø> (ø)
src/EllCrv/EllCrv.jl 95.04% <75.00%> (-0.71%) ⬇️
src/EllCrv/Isomorphisms.jl 89.61% <0.00%> (-1.25%) ⬇️

... and 27 files with indirect coverage changes

@thofma thofma enabled auto-merge (squash) November 9, 2024 10:07
@thofma
Copy link
Owner

thofma commented Nov 9, 2024

This 'breaks' the booktests, since the output changed (https://github.com/thofma/Hecke.jl/actions/runs/11754631063/job/32757483758?pr=1677#step:7:19595). So this change becomes breaking unfortunately. I will label it as such and we merge it once we want to make a breaking release.

@thofma thofma added the breaking label Nov 9, 2024
@StevellM
Copy link
Collaborator

@simonbrandhorst Why don't you make the base field part of the detailed printing ? Since a priori one could define elliptic curves over any (nice?) field, wouldn't it sense to tell whether it is defined over $\mathbb{Q}$ or $\mathbb{F}_{121}$.

@simonbrandhorst
Copy link
Collaborator Author

We could do that. But I don't want to fix the doctests again xD.

@thofma
Copy link
Owner

thofma commented Nov 11, 2024

What would the right order be? First the equation or first the base field?

@simonbrandhorst
Copy link
Collaborator Author

I would put the base field first.

Affine algebraic set
  in affine 2-space over QQ with coordinates [x1, x2]
defined by ideal (x1*x2)

@thofma
Copy link
Owner

thofma commented Nov 11, 2024

How about the following? If we agree, I can push the changes.

julia> E = elliptic_curve(GF(11), [1, 2])
Elliptic curve
  over prime field of characteristic 11
with equation
  y^2 = x^3 + x + 2

julia> E = elliptic_curve(K, [1, 2])
Elliptic curve
  over number field of degree 4 over QQ
with equation
  y^2 = x^3 + x + 2

and

julia> [E]
1-element Vector{EllipticCurve{AbsSimpleNumFieldElem}}:
 Elliptic curve over K with equation y^2 = x^3 + x + 2

@thofma thofma disabled auto-merge November 11, 2024 19:20
@StevellM
Copy link
Collaborator

It looks good like that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants