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] - refactor(analysis/normed_space): merge normed_space and semi_normed_space #8218

Closed
wants to merge 32 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 7, 2021

There are no theorems which are true for [normed_group M] [normed_space R M] but not [normed_group M] [semi_normed_space R M]; so there is about as much value to the semi_normed_space / normed_space distinction as there was between module / semimodule. Since we eliminated semimodule, we should eliminte semi_normed_space too.

This relaxes the typeclass arguments of normed_space to make it a drop-in replacement for semi_normed_space; or viewed another way, this removes normed_space and renames semi_normed_space to replace it.

This does the same thing to normed_algebra and seminormed_algebra, but these are hardly used anyway.

Zulip

As with any typeclass refactor, this randomly breaks elaboration in a few places; presumably, it was able to unify arguments from one particular typeclass path, but not from another. To fix this, some type ascriptions have to be added where previously none were needed. In a few rare cases, the elaborator gets so confused that we have to enter tactic mode to produce a term.

This isn't really a new problem - this PR just makes these issues all visible at once, whereas normally we'd only run into one or two at a time. Optimistically Lean 4 will fix all this, but we won't really know until we try.


Open in Gitpod

@eric-wieser
Copy link
Member Author

eric-wieser commented Jul 7, 2021

For reasons I don't understand, this change breaks:

example (𝕜 ι : Type*) (E : ι → Type*)
  [nondiscrete_normed_field 𝕜] [Π i, normed_group (E i)] [Π i, normed_space 𝕜 (E i)] :
  Π i, module 𝕜 (E i) := by apply_instance

but this works fine

example (𝕜 ι : Type*) (E : ι → Type*)
  [nondiscrete_normed_field 𝕜] [Π i, normed_group (E i)] [Π i, normed_space 𝕜 (E i)] :
  Π i, module 𝕜 (E i) := λ _, by apply_instance

I'm increasingly of the opinion that mixin typeclasses are broken in nasty ways in Lean 3; this is the dfinsupp problem all over again, and has come up multiple times on Zulip.

Edit: I found a workaround which I posted in this Zulip thread, and have committed it. This problem is not a new breakage, it already existed for semi_normed_space.

@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes label Jul 7, 2021
@github-actions github-actions bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Jul 7, 2021
Copy link
Member

@riccardobrasca riccardobrasca left a comment

Choose a reason for hiding this comment

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

LGTM! And it's very nice we can remove some @!

@eric-wieser
Copy link
Member Author

Unfortunately there's a timeout in banach.lean. I'm hoping that I can get away with removing the @s (#8221) before making this refactor, and have an idea to speed up the banach proof using #8220

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Aug 9, 2021
@eric-wieser eric-wieser force-pushed the eric-wieser/remove-seminormed-space branch from a0feba4 to bf3bc33 Compare August 10, 2021 12:21
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Aug 10, 2021
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jan 12, 2022
@eric-wieser eric-wieser force-pushed the eric-wieser/remove-seminormed-space branch from 13012d7 to a46da74 Compare January 13, 2022 13:58
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jan 13, 2022
@leanprover-community-bot-assistant leanprover-community-bot-assistant removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Jan 14, 2022
@leanprover-community leanprover-community deleted a comment from github-actions bot Jan 14, 2022
@eric-wieser
Copy link
Member Author

It took 6 months, but all the timeouts are gone!

@eric-wieser eric-wieser added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 14, 2022
Comment on lines +241 to +245
/-- A special case of `pi.algebra` for non-dependent types. Lean get stuck on the definition
below without this. -/
instance _root_.function.algebra (I : Type*) {R : Type*} (A : Type*) {r : comm_semiring R}
[semiring A] [algebra R A] : algebra R (I → A) :=
pi.algebra _ _
Copy link
Member Author

Choose a reason for hiding this comment

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

I introduced function.has_scalar and function.smul_comm_class in another recent PR, and discussed with @madvorak about adding function.has_add etc to fix a similar problem - I'd prefer to leave collecting all those instances together into the right place to a follow-up PR.

@eric-wieser eric-wieser force-pushed the eric-wieser/remove-seminormed-space branch from d362dad to c0be454 Compare January 15, 2022 18:46
@eric-wieser
Copy link
Member Author

I plan to come back to this and check whether those last two commits remove the need for some of the downstream changes

@eric-wieser eric-wieser force-pushed the eric-wieser/remove-seminormed-space branch from 33bde92 to 919347c Compare January 16, 2022 11:00
@riccardobrasca
Copy link
Member

Looks good to me, and since it compiles it is probably a good idea to merge it soon.

@RemyDegenne
Copy link
Collaborator

After all your work to get it to compile, the final diff is rather small in the end :) . It looks good to me!
bors r+

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Jan 16, 2022
bors bot pushed a commit that referenced this pull request Jan 16, 2022
…d_space` (#8218)

There are no theorems which are true for `[normed_group M] [normed_space R M]` but not `[normed_group M] [semi_normed_space R M]`; so there is about as much value to the `semi_normed_space` / `normed_space` distinction as there was between `module` / `semimodule`. Since we eliminated `semimodule`, we should eliminte `semi_normed_space` too.

This relaxes the typeclass arguments of `normed_space` to make it a drop-in replacement for `semi_normed_space`; or viewed another way, this removes `normed_space` and renames `semi_normed_space` to replace it.

This does the same thing to `normed_algebra` and `seminormed_algebra`, but these are hardly used anyway.

[Zulip](https://leanprover.zulipchat.com/#narrow/stream/113488-general/topic/semi_normed_space.20vs.20normed_space/near/245089933)

As with any typeclass refactor, this randomly breaks elaboration in a few places; presumably, it was able to unify arguments from one particular typeclass path, but not from another. To fix this, some type ascriptions have to be added where previously none were needed. In a few rare cases, the elaborator gets so confused that we have to enter tactic mode to produce a term.

This isn't really a new problem - this PR just makes these issues all visible at once, whereas normally we'd only run into one or two at a time. Optimistically Lean 4 will fix all this, but we won't really know until we try.
@bors
Copy link

bors bot commented Jan 16, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(analysis/normed_space): merge normed_space and semi_normed_space [Merged by Bors] - refactor(analysis/normed_space): merge normed_space and semi_normed_space Jan 16, 2022
@bors bors bot closed this Jan 16, 2022
@bors bors bot deleted the eric-wieser/remove-seminormed-space branch January 16, 2022 15:00
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

4 participants