-
Notifications
You must be signed in to change notification settings - Fork 132
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
Conversation
Codecov Report
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
|
There was a problem hiding this 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.
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") |
7a60b06
to
ea60846
Compare
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 |
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"?
- fix error because `Indent` was undefined - remove extra newline at end - add a missing `Lowercase` - simplify the code
ea60846
to
bfa0a89
Compare
There was a problem hiding this 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
- 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)
### 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
Indent
was undefinedLowercase