-
Notifications
You must be signed in to change notification settings - Fork 316
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
feat(RingTheory/LaurentSeries): add notation #16639
base: master
Are you sure you want to change the base?
Conversation
PR summary 27c38d0421Import changes for modified filesNo significant changes to the import graph Import changes for all files
|
I certainly like the look of this. The notation is standard, even to the extent that the brackets are written close together in LaTeX. Happy to hear another opinion but LGTM. |
This is something that we discussed with @mariainesdff and we ended up wondering if this notation should be used for Laurent Series as |
I don't know enough to have any opinion clearly so I'm happy to close if that's recommended! |
After thinking carefully for a while, I am also in favour of the changes proposed by this PR. @Julian if you can update it, I will be happy to have a final look and suggest that it be merged. |
@faenuccio I don't think there seem to be any merge conflicts -- are you asking me to rebase it anyways? (Happy to do whatever is helpful). Also could you perhaps say anything about what changed your mind, just curious to learn! |
A couple of more PR included Laurent Series (in particular #16865, that is being merged) and it would be worthwhile to recheck for occurrences of LaurentSeries * in Mathlib to replace them with your notation.
Ah sure, thanks for asking. Mainly three points:
I am sorry if these motivations were already perfectly clear in your mind, it just took me a while to convince myself 😢 |
Aha! OK, happy to. Will ping you when I've done so (after your PR is merged).
Nothing is very clear in my mind :D, though thankfully I at least understood some of what you said! Thank you for elaborating! |
9959af2
to
fe559e2
Compare
OK this is ready for review I believe @faenuccio. |
Thanks, having a look! |
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.
Thanks! I think that you can also replace the notation in the docstring (see line 282, for instance, in the dostring of def coeAlgHom
(it also seems to me that there is a repetition three lines below). Also line 293 does not have this notation, can you check again the document? BTW, I think (please check) that we also have R(T)
for rational functions, and it would be worth it to enforce also that notation in this file.
Thanks for the review! That section (from line 278 till 413) is not in the Or is there a way to be able to use it in the Lean code too? If I I'll have a look for the rational function notation. |
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.
Thanks for the review! That section (from line 278 till 413) is not in the
LaurentSeries
namespace, it's in theRatFunc
one, so the notation doesn't work in it, which was why I didn't make the change there -- obviously I can do it in the docstring anyways if that's preferred?Or is there a way to be able to use it in the Lean code too? If I
open LaurentSeries
that will open everything, no, not just enable the notation?I'll have a look for the rational function notation.
In order to only open notation you can write
open scoped LaurentSeries
(for instance right after the namespace RatFunc
on line 280). This will allow the scoped
(sic!) notation to fire, without carrying over all results in the namespace.
Thanks for that tip, I'm sure I've seen it before and forgot what it does. It seems to work perfectly. I've made the changes, and also made the changes to use the PowerSeries notation in the file -- I'm still looking for the notation for rational functions, I don't see it immediately yet. Right now the only places without the notation then are in the module docstring -- let me know if I should change it there too but I followed what the PowerSeries module did (which is to use it in the docstring). |
Concerning |
There is one place that uses the notation in a docstring, that place is here (but yeah it seems not to exist, maybe due to those old Zulip threads about how safe it was to have notation with single parentheses? I can't find them via Zulip search). I am happy to make the docstring notation change of course -- but do you want me to make it to the PowerSeries file as well in this same PR? It too doesn't mention the notation in the docstring. Or should I do that in a follow-up? |
There are a bunch of places in the PowerSeries files that don't use the PowerSeries notation. To me it seems best to do it in a follow up along with fixing those. I'll proceed under that assumption unless you tell me otherwise! |
Both are OK, so go ahead as you see fit. Final question before sending this PR to merge: have you checked that |
2f4ecfa
to
8f9922c
Compare
OK I've added the notation to the module docstring and type docstring as you suggested, so I think this is ready now!
Yes! At least via a grep, this is all of them. |
Awesome, thanks so much for your work! |
maintainer merge |
🚀 Pull request has been placed on the maintainer queue by faenuccio. |
Refs: leanprover/vscode-lean4#523
I've only made use of the notation in the file in places that were already within the namespace, but happy to make further changes if preferred.