-
Notifications
You must be signed in to change notification settings - Fork 126
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
Adjust to AA/Nemo/Hecke #3288
Adjust to AA/Nemo/Hecke #3288
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Renaming script used for the first commit:
|
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 noticed some printing things that don't look good (see comments). This is not a blocker for this PR, but should be adapted in a timely manner (for the book).
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3288 +/- ##
==========================================
- Coverage 81.64% 81.62% -0.02%
==========================================
Files 546 546
Lines 73445 73465 +20
==========================================
+ Hits 59964 59969 +5
- Misses 13481 13496 +15
|
As already discussed in Slack: there is a problem in the doctests now because a cached polynomial ring keeps whatever name it was assigned to first.
In the last map, it still says |
For me, it's fine to merge #3294. Sorting the printing out will take a moment here and it is no problem to run the renaming script again. |
Regarding the "cached name": I actually wanted to fix that a year ago or so; the idea being that the logic should not store/cache the REPL name of an object, but always query it dynamically (this has a performance penalty but printing is slow anyway). But I failed to untangle the messy relation ship between |
+1 for documenting all the printing magic that is happening! |
@fieker: Do you have an opinion (since this is your invention). The option is
At the moment we do 1), but @fingolfin/@lgoettgens propose to change it to 2). |
To keep the discussion at a better place, I created Nemocas/AbstractAlgebra.jl#1594 with my proposal and some more explanation and motivation. Please put your opinions there instead |
Not storing won't help. The docstrings possibly set up the same ring with different names. Otherwise: whatever you orefer |
Maybe in the docstrings the ring should R always? |
Different doctests run in different modules, so the find_name won't find names from other docstrings. I am very certain that this helps, but I'll verify before merging anything.
I agree, but that's the current state
This just fixes symptoms. For other types that may use this feature (but we haven't experienced the issues yet) there may be no canonical name to choose. And this would be very hard to enforce. Thanks for your words, I'll then proceed with |
* Adjust to AA/Nemo/Hecke * `haspreimage`/`has_preimage` -> `has_preimage_with_preimage` * Readd special show functions for hom/direct_product/... * Put specialized show methods in a file that's actually included
Requires oscar-system/Singular.jl#767