-
Notifications
You must be signed in to change notification settings - Fork 381
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(AlgebraicGeometry): Introduce Scheme.Opens
.
#15001
Conversation
PR summary 5916387248Import changes for modified filesNo significant changes to the import graph Import changes for all files
|
!bench |
Here are the benchmark results for commit 89bba21. |
!bench |
Here are the benchmark results for commit 2fc3307. Benchmark Metric Change
==============================================================================
- build linting 6.4%
- ~Mathlib.AlgebraicGeometry.AffineScheme instructions 22.5%
+ ~Mathlib.AlgebraicGeometry.Morphisms.Basic instructions -46.9%
+ ~Mathlib.AlgebraicGeometry.Morphisms.RingHomProperties instructions -50.6%
+ ~Mathlib.AlgebraicGeometry.Restrict instructions -59.5%
+ ~Mathlib.AlgebraicGeometry.Scheme instructions -22.4% |
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.
I'll review more tomorrow.
def Scheme.openCoverOfSuprEqTop {s : Type*} (X : Scheme.{u}) (U : s → X.Opens) | ||
(hU : ⨆ i, U i = ⊤) : X.OpenCover where |
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.
def Scheme.openCoverOfSuprEqTop {s : Type*} (X : Scheme.{u}) (U : s → X.Opens) | |
(hU : ⨆ i, U i = ⊤) : X.OpenCover where | |
def Scheme.openCoverOfiSupEqTop {s : Type*} (X : Scheme.{u}) (U : s → X.Opens) | |
(hU : ⨆ i, U i = ⊤) : X.OpenCover where |
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.
This is copy pasted from elsewhere. I'll make this change in a follow-up PR.
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.
It looks like it does not make anything worse and some things definitely look better. The old X ∣_ᵤ U
looked a bit funky in some places. Also that we can use dot notation on the open set is nice and opens up a potential refactor of IsAffineOpen
.
instance ΓRestrictAlgebra {X : Scheme.{u}} (U : X.Opens) : | ||
Algebra (Scheme.Γ.obj (op X)) (Scheme.Γ.obj (op U)) := | ||
(Scheme.Γ.map U.ι.op).toAlgebra |
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.
instance ΓRestrictAlgebra {X : Scheme.{u}} (U : X.Opens) : | |
Algebra (Scheme.Γ.obj (op X)) (Scheme.Γ.obj (op U)) := | |
(Scheme.Γ.map U.ι.op).toAlgebra | |
instance ΓRestrictAlgebra {X : Scheme.{u}} (U : X.Opens) : | |
Algebra Γ(X, ⊤) Γ(U, ⊤) := | |
(Scheme.Γ.map U.ι.op).toAlgebra |
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.
This change will break other things. I'll make this change in a follow-up PR.
Co-authored-by: Christian Merten <[email protected]>
Co-authored-by: Christian Merten <[email protected]>
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! LGTM.
rw [Scheme.comp_app, Scheme.comp_app, ΓSpecIso_obj_hom, | ||
Scheme.ofRestrict_app_self] | ||
simp only [Category.assoc] | ||
dsimp only [asIso_inv, Functor.op_obj, unop_op] | ||
rw [← Functor.map_comp_assoc, ← op_comp, eqToHom_trans, Scheme.eq_restrict_presheaf_map_eqToHom, | ||
rw [Scheme.comp_app, Scheme.comp_app, ΓSpecIso_obj_hom, Scheme.Opens.ι_app_self] | ||
simp only [asIso_inv, Scheme.comp_coeBase, Opens.map_comp_obj, Scheme.Opens.topIso_inv, | ||
Opens.map_top, Functor.id_obj, Functor.comp_obj, | ||
Functor.rightOp_obj, Scheme.Γ_obj, unop_op, Scheme.Spec_obj, Scheme.Opens.topIso_hom, | ||
Category.assoc] | ||
rw [← Functor.map_comp_assoc, ← op_comp, eqToHom_trans, Scheme.Opens.eq_presheaf_map_eqToHom, |
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.
The slowdown on this file comes from this proof and it will be improved in a later PR where the unit of the gamma-spec adjunction is specialized and be provided dedicated API.
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.
This whole proof can be solved by a single simp only [many things are here]
and the have
can be deleted.
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.
I am guessing the rw
are unfolding things at default transparency that are very unpleasant. At least with simp
, you can control the unfolding a bit more
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.
Do you mean introducing intermediate lemmas? I'm not sure how to make this work.
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.
Okay I managed to condense it into two blobs of simp and removed the nolint and the maxheartbeat.
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.
Hmm. I think it is supposed to operate at reducible
but I can't tell if it is doing anything atm
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.
The actual substitution via kabstract
is done at reducible
transparency but there is at least one other isDefEq
call that is done at default
. So we start unfolding these more than is desired here
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.
Is that intended? If this happens to rw
everywhere, it sounds like a performance hazard to me.
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.
With the huge caveat that I just looked at the file for a couple of minutes, no.
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.
I'll check on this at office hours tomorrow at the latest
…lib4 into erd1/schemeOpens2
You might be able to improve your performance by removing the attribute making |
!bench |
Here are the benchmark results for commit 5916387. Benchmark Metric Change
==================================================================
+ ~Mathlib.AlgebraicGeometry.AffineScheme instructions -35.1%
+ ~Mathlib.AlgebraicGeometry.Morphisms.Basic instructions -46.0%
+ ~Mathlib.AlgebraicGeometry.Restrict instructions -59.2%
+ ~Mathlib.AlgebraicGeometry.Scheme instructions -21.7% |
Ready to go? |
Yes. Thanks! The improvements mentioned above will come in a later PR. |
bors merge |
Introduced `Scheme.Opens` as the type of open sets of a scheme. Provided `instance : CoeOut X.Opens Scheme` to replace the notation `X ∣_ᵤ U`, and renamed `Scheme.ιOpens` to `Scheme.Opens.ι` to enable dot notation. Co-authored-by: Andrew Yang <[email protected]>
Pull request successfully merged into master. Build succeeded: |
Scheme.Opens
.Scheme.Opens
.
Introduced
Scheme.Opens
as the type of open sets of a scheme. Providedinstance : CoeOut X.Opens Scheme
to replace the notationX ∣_ᵤ U
, and renamedScheme.ιOpens
toScheme.Opens.ι
to enable dot notation.Main changes are in
Restrict.lean
.