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

[Merged by Bors] - chore(data/mv_polynomial): Rename variables #4208

Closed
wants to merge 8 commits into from

Conversation

shingtaklam1324
Copy link
Collaborator

@shingtaklam1324 shingtaklam1324 commented Sep 22, 2020

I renamed α to R throughout. I also changed the \sigma to σ in basic.lean, see leanprover-community/doc-gen#62


Formerly: The file used α and β as the semirings, not R and S as suggested by the original module doc string.

@shingtaklam1324 shingtaklam1324 added the awaiting-review The author would like community review of the PR label Sep 22, 2020
@jcommelin
Copy link
Member

While I agree that this should be fixed, I would rather see that the docstring stays the same and the variables in the file get updated to "meaninful" variable names. That seems to be the tendency in mathlib.
If you don't have energy for that refactor right now, then I'm happy to merge this, because consistency is more important than progress (-;

@shingtaklam1324
Copy link
Collaborator Author

shingtaklam1324 commented Sep 22, 2020

I have a week or two before term so I'm happy to fix it if you prefer all the mv_polynomial files used R as the (semi)ring. It's much easier to change α to R than the other way round ;)

The ones that use α are basic.lean, comm_ring.lean, equiv.lean, rename.lean and variables.lean. My only concern right now is that there is 5 open PRs for mv_polynomials, I don't think merge conflicts will be a big issue but I'm not sure.

@jcommelin
Copy link
Member

Of those five files, one of my PRs fixes rename.lean. I think I don't touch comm_ring.lean at all. For the rest... we'll fix merge conflicts when we see them (-;

@shingtaklam1324
Copy link
Collaborator Author

Alright then. I won't touch rename.lean but I'll change the rest.

@shingtaklam1324 shingtaklam1324 added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Sep 22, 2020
@shingtaklam1324
Copy link
Collaborator Author

shingtaklam1324 commented Sep 22, 2020

I did a search-and-replace changing all the αs to Rs. Hopefully I didn't break anything.

Question: There are lemmas which have other (semi)rings β, γ, δ etc. Should I change β => S, γ => T, δ => U? Or should I use other letters (if S, T, U have any particular meanings?)

@shingtaklam1324 shingtaklam1324 changed the title chore(data/mv_polynomial/basic): Fix module doc string chore(data/mv_polynomial/basic): Rename variables Sep 22, 2020
@jcommelin
Copy link
Member

Yes, that looks good to me. Other common letters for (commutative) rins/algebras are A, B, C

@jcommelin
Copy link
Member

Hmm, sorry, addition: I think I would rather use R₁ and R₂ or S₁ etc, instead of T or U... those are not so common letters for rings.

@shingtaklam1324
Copy link
Collaborator Author

Thanks. I'll change those once I figure out why the build broke.

aeval f (bind₂ g φ) = eval₂_hom ((@aeval σ S T f _ _ _ : mv_polynomial σ S →+* T).comp g) f φ :=
aeval f (bind₂ g φ) = eval₂_hom ((@aeval S σ _ T f _ _ : mv_polynomial σ S →+* T).comp g) f φ :=
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit annoying, (and I suspect a lot more has been broken). The order of the arguments has been changed in aeval by my renaming.

Copy link
Member

Choose a reason for hiding this comment

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

Right... this is exactly why we would like to have all the variables renamed, because they were used with very different meanings in different declarations.

Comment on lines 821 to 822
variables {R : Type u} {A : Type v} {S : Type w} (f : σ → A)
variables [comm_semiring R] [comm_semiring A] [algebra R A] [comm_semiring S]
variables {A : Type v} {S : Type w} (f : σ → A)
variables [comm_semiring A] [algebra R A] [comm_semiring S]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reason for the above breakage is this change. As R was already being used as a variable in this part of the file, when I changed α to R, Lean complains that R already exists, so I deleted the R from this sections. This then changed the order of arguments for aeval

Copy link
Member

Choose a reason for hiding this comment

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

Ok... It would be good to keep the order of variables as much the same as possible. (Otherwise this refactor might ramify into many other files, which seems undesirable.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually the build succeeded, so there was only one breakage. I think if I rename β, γ, δ etc. then I can get rid of the A on L821 and the type arguments should be together, and the typeclass ones together. The only change would then be σ S being changed to S σ in the previous comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wait, the previous comment isn't right. comm_semiring R will still come before f.

The easiest way forward is just to use the original variables but with new names. Then nothing should be broken. But I'm not sure if that's the right way.

Copy link
Member

Choose a reason for hiding this comment

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

I'll let you do what you think is best (-;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright thanks. If there's only one breakage in mathlib then I'm not too concerned about it to be perfectly honest.

@shingtaklam1324 shingtaklam1324 added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Sep 22, 2020
@shingtaklam1324 shingtaklam1324 changed the title chore(data/mv_polynomial/basic): Rename variables chore(data/mv_polynomial): Rename variables Sep 22, 2020
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors merge

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Sep 22, 2020
bors bot pushed a commit that referenced this pull request Sep 22, 2020
I renamed `α` to `R` throughout. I also changed the `\sigma` to `σ` in `basic.lean`, see leanprover-community/doc-gen#62
@bors
Copy link

bors bot commented Sep 22, 2020

This PR was included in a batch that was canceled, it will be automatically retried

bors bot pushed a commit that referenced this pull request Sep 22, 2020
I renamed `α` to `R` throughout. I also changed the `\sigma` to `σ` in `basic.lean`, see leanprover-community/doc-gen#62
@bors
Copy link

bors bot commented Sep 22, 2020

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 22, 2020
I renamed `α` to `R` throughout. I also changed the `\sigma` to `σ` in `basic.lean`, see leanprover-community/doc-gen#62
@bors
Copy link

bors bot commented Sep 22, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(data/mv_polynomial): Rename variables [Merged by Bors] - chore(data/mv_polynomial): Rename variables Sep 22, 2020
@bors bors bot closed this Sep 22, 2020
@bors bors bot deleted the fix-mv-poly-doc branch September 22, 2020 15:46
@YaelDillies YaelDillies removed the awaiting-review The author would like community review of the PR label Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants