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] - feat: port FieldTheory.Ratfunc #4293

Closed
wants to merge 30 commits into from

Conversation

adomani
Copy link
Collaborator

@adomani adomani commented May 24, 2023

I am going to try to work on this for a bit, but it looks hard.


Open in Gitpod

Mathbin -> Mathlib
fix certain import statements
move "by" to end of line
add import to Mathlib.lean
@adomani adomani added WIP Work in progress mathlib-port This is a port of a theory file from mathlib. labels May 24, 2023
@adomani
Copy link
Collaborator Author

adomani commented May 24, 2023

I am stopping for now: feel free to step in!

@adomani adomani added help-wanted The author needs attention to resolve issues and removed WIP Work in progress labels May 24, 2023
@Vierkantor
Copy link
Contributor

Let me look at this for an hour or two...

@adomani
Copy link
Collaborator Author

adomani commented May 25, 2023

@Vierkantor this is great, thanks! I am inching forward with removing errors!

@adomani
Copy link
Collaborator Author

adomani commented May 25, 2023

Ok, I am making progress.

I unblocked a large chunk by removing the CommRing K, IsDomain K assumptions, when Field K was also present: I think that, while the omittable assumptions, might get omitted after parsing, but they mess up the proof, since they get entangled with the terms.

@adomani
Copy link
Collaborator Author

adomani commented May 25, 2023

I am taking a break from this file.

There are still a few issues:

  • liftRingHom just does not want to be proved by me;
  • the inv_zero field of instance : Field (Ratfunc K) does not yield to frac_tac;
  • num_div_denom is stopped;
  • several norm_cast lemmas have a coe on the RHS (and two have broken proofs).

I suspect that some of these problems might be caused by the omit/include issue that I mentioned above. I will try to produce a #mwe to see if this is really an issue or whether I am mistaken.

Also, it would be good to check that the lemmas that now assume Field K really need it! My mathlib3 broke mid-port and I did not fix it, since I felt that I was on a roll with this file!

@Vierkantor
Copy link
Contributor

several norm_cast lemmas have a coe on the RHS (and two have broken proofs).

It feels like coe_C and coe_X are false positives: the coe on the RHS applies the bundled homs, so morally I'd say it does not coerce from one type to another.

@adomani
Copy link
Collaborator Author

adomani commented May 25, 2023

Well done, @Vierkantor!!

Is there an "autofix docstrings" script?

I also have a something to say about the choice of x as a symbol for a generic term in Ratfunc K and also defining X : Ratfunc K... 😄

@Vierkantor
Copy link
Contributor

Is there an "autofix docstrings" script?

Yes! ./scripts/fix-comments.py. But I think we should first go through the naming conventions before we do this.

I also have a something to say about the choice of x as a symbol for a generic term in Ratfunc K and also defining X : Ratfunc K... smile

Yeah, some (variable) names are not optimal but let's postpone that change until the port is complete.

Should Ratfunc really be RatFunc? How do you change the name of the file, so that it is picked up by the automation?

I think we can just git mv Mathlib/FieldTheory/Ratfunc.lean Mathlib/FieldTheory/RatFunc.lean, because the file docstring includes the mathlib3 source files.

@adomani
Copy link
Collaborator Author

adomani commented May 25, 2023

Also, I think that after fighting the non-existence of omit/include, the end result no longer needs the sectioning: after Reid's talk, I'll try to reduce the diff.

@adomani
Copy link
Collaborator Author

adomani commented May 25, 2023

@Vierkantor, I "reverted" the section change: after the fact, Lean can figure out everything it seems. It was just confusing me in the course of the proof.

As for the naming convention, I am not too familiar with the capitalization rules: can someone else (maybe you? 👼 ) help with that?

@adomani adomani added awaiting-review The author would like community review of the PR and removed help-wanted The author needs attention to resolve issues labels May 25, 2023
Mathlib/FieldTheory/Ratfunc.lean Outdated Show resolved Hide resolved
Mathlib/FieldTheory/Ratfunc.lean Outdated Show resolved Hide resolved
Mathlib/FieldTheory/Ratfunc.lean Outdated Show resolved Hide resolved
@Vierkantor
Copy link
Contributor

As for the naming convention, I am not too familiar with the capitalization rules: can someone else (maybe you? angel ) help with that?

Doing that right now!

Vierkantor and others added 4 commits May 25, 2023 19:21
@Vierkantor
Copy link
Contributor

Done, I think this is ready for review again.

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

@semorrison semorrison added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels May 25, 2023
bors bot pushed a commit that referenced this pull request May 25, 2023
I am going to try to work on this for a bit, but it looks hard.



Co-authored-by: Chris Hughes <chrishughes24@gmail.com>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
Co-authored-by: Vierkantor <vierkantor@vierkantor.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@bors
Copy link

bors bot commented May 25, 2023

Build failed:

@Vierkantor
Copy link
Contributor

Let's try again (with @jcommelin's verbal approval)!

bors r+

bors bot pushed a commit that referenced this pull request May 25, 2023
I am going to try to work on this for a bit, but it looks hard.



Co-authored-by: Chris Hughes <chrishughes24@gmail.com>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
Co-authored-by: Vierkantor <vierkantor@vierkantor.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@bors
Copy link

bors bot commented May 25, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title feat: port FieldTheory.Ratfunc [Merged by Bors] - feat: port FieldTheory.Ratfunc May 25, 2023
@bors bors bot closed this May 25, 2023
@bors bors bot deleted the port/FieldTheory.Ratfunc branch May 25, 2023 22:41
@urkud urkud mentioned this pull request May 31, 2023
qawbecrdtey pushed a commit to qawbecrdtey/greedoid-mathlib4 that referenced this pull request Jun 12, 2023
I am going to try to work on this for a bit, but it looks hard.



Co-authored-by: Chris Hughes <chrishughes24@gmail.com>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
Co-authored-by: Vierkantor <vierkantor@vierkantor.com>
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mathlib-port This is a port of a theory file from mathlib. ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants