Skip to content

[Merged by Bors] - feat: Rat-dependent norm_num functionality#1441

Closed
thorimur wants to merge 20 commits intomasterfrom
rat-norm_num
Closed

[Merged by Bors] - feat: Rat-dependent norm_num functionality#1441
thorimur wants to merge 20 commits intomasterfrom
rat-norm_num

Conversation

@thorimur
Copy link
Copy Markdown
Contributor

@thorimur thorimur commented Jan 9, 2023

As per #1102, we extend norm_num to handle Rats.

We leave most of the theorems sorried, but the evaluation and tests work!

@thorimur thorimur added WIP Work in progress t-meta Tactics, attributes or user commands labels Jan 9, 2023
@thorimur thorimur linked an issue Jan 9, 2023 that may be closed by this pull request
@thorimur thorimur added awaiting-review WIP Work in progress and removed WIP Work in progress awaiting-review labels Jan 13, 2023
@thorimur thorimur added awaiting-review and removed WIP Work in progress labels Jan 14, 2023
Comment on lines +397 to +398
attribute [nolint defLemma] isRat_add isRat_neg isRat_sub isRat_mul isRat_pow isRat_inv_pos
isRat_inv_neg isRat_div -- FIXME: figure out how to get the linter to work with these
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
attribute [nolint defLemma] isRat_add isRat_neg isRat_sub isRat_mul isRat_pow isRat_inv_pos
isRat_inv_neg isRat_div -- FIXME: figure out how to get the linter to work with these

Maybe IsRat should be in Prop, just like IsInt. I don't know if we ever need to extract the inverse of the denominator.

But if IsRat needs to stay in Prop, then these should all be defs and not theorems. So either way, the nolint needs to be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried to change as many to def as possible, but isRat_pow gives an error like "unsupported Mathlib.Meta.NormNum.IsNat.casesOn application during code generation" when changed to def, as do the ones in NormNum.Core. What's the fix for this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fix is to move IsRat into Prop. Nothing seems to break because of that and all the theorems can remain theorems. (Sorries in defs are a big no-no, this will break mathport.)

inductive IsRat [Ring α] (a : α) (num : ℤ) (denom : ℕ) : Prop
  | mk (inv : Invertible (denom : α)) (eq : a = num * ⅟(denom : α))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was waiting for the green check to reply but looks like you beat me to it! Great! :)

@gebner
Copy link
Copy Markdown
Member

gebner commented Jan 14, 2023

Since this is blocking a lot, let's merge it with sorries.

bors r+

bors bot pushed a commit that referenced this pull request Jan 14, 2023
As per #1102, we extend `norm_num` to handle `Rat`s.

We leave most of the theorems sorried, but the evaluation and tests work!

Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Jan 14, 2023
@bors
Copy link
Copy Markdown

bors bot commented Jan 14, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: Rat-dependent norm_num functionality [Merged by Bors] - feat: Rat-dependent norm_num functionality Jan 14, 2023
@bors bors bot closed this Jan 14, 2023
@bors bors bot deleted the rat-norm_num branch January 14, 2023 01:54
jcommelin pushed a commit that referenced this pull request Jan 23, 2023
As per #1102, we extend `norm_num` to handle `Rat`s.

We leave most of the theorems sorried, but the evaluation and tests work!

Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rat-dependent norm_num functionality

3 participants