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

Fix show for RelativeBrauerGroupElem #3373

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

fingolfin
Copy link
Member

  • fix error because Indent was undefined
  • remove extra newline at end
  • add a missing Lowercase
  • simplify the code

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Merging #3373 (8d8131a) into master (138d8d8) will decrease coverage by 0.01%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3373      +/-   ##
==========================================
- Coverage   82.08%   82.08%   -0.01%     
==========================================
  Files         556      557       +1     
  Lines       74074    73973     -101     
==========================================
- Hits        60807    60719      -88     
+ Misses      13267    13254      -13     
Files Coverage Δ
experimental/GModule/GaloisCohomology.jl 65.19% <100.00%> (+1.33%) ⬆️

... and 31 files with indirect coverage changes

Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

Can you maybe add a doctest to just any function exercising this show? This would help to find future similar things and increases coverage.

@fingolfin fingolfin added the backport 1.0.x Should be backported to the release 1.0 branch label Feb 15, 2024
@fingolfin
Copy link
Member Author

I'll see what I can do.

Added "backport to 1.0 label" as I I need this for the book (where it is "tested")

@fingolfin fingolfin force-pushed the mh/fix-RelativeBrauerGroupElem-printing branch 2 times, most recently from 7a60b06 to ea60846 Compare February 15, 2024 15:18
Element of relative Brauer group of number field of degree 1 over QQ
<2, 2> -> 1//2 + Z
Complex embedding of number field -> 1//2 + Z
<5, 5> -> 0 + Z
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems the order in which these three lines are printed is not stable, hence some of the doctest CI jobs fail :/

Copy link
Member

Choose a reason for hiding this comment

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

Is there a canonical way to sort them (for a consistent printing)? If not, you might use a doctest-filter

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 don't see a good way to sort the keys in general, they are ideals in a number field, or field embeddings. But we could make sure that the ideals come first. And perhaps the ideals could be sorted by minimum or norm. That won't give a unique sorting in all cases, but it is good enough for this example and it probably is nice for the user overall. Will do that for now.

@fieker I also wonder if we should print something nicer instead of "Complex embedding of number field"?

@benlorenz benlorenz mentioned this pull request Feb 16, 2024
33 tasks
- fix error because `Indent` was undefined
- remove extra newline at end
- add a missing `Lowercase`
- simplify the code
@fingolfin fingolfin force-pushed the mh/fix-RelativeBrauerGroupElem-printing branch from ea60846 to bfa0a89 Compare February 16, 2024 13:46
Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

I have no idea of the mathematics behind this, but the printing change by itself look good

@fingolfin fingolfin merged commit aefcd0a into master Feb 19, 2024
24 checks passed
@fingolfin fingolfin deleted the mh/fix-RelativeBrauerGroupElem-printing branch February 19, 2024 10:23
aaruni96 pushed a commit that referenced this pull request Feb 19, 2024
- fix error because `Indent` was undefined
- remove extra newline at end
- add a missing `Lowercase`
- simplify the code
- make output order less random

(cherry picked from commit aefcd0a)
@aaruni96 aaruni96 removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 19, 2024
benlorenz added a commit that referenced this pull request Feb 23, 2024
### Backported PRs

- [x] #3367
- [x] #3379 
- [x] #3369
- [x] #3291
- [x] #3325
- [x] #3350 
- [x] #3351
- [x] #3365 
- [x] #3366
- [x] #3382
- [x] #3373
- [x] #3341
- [x] #3346
- [x] #3381
- [x] #3385
- [x] #3387 
- [x] #3398 
- [x] #3399 
- [x] #3374 
- [x] #3406 
- [x] #2823
- [x] #3298
- [x] #3386 
- [x] #3412 
- [x] #3392 
- [x] #3415 
- [x] #3394
- [x] #3391
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.

3 participants