-
Notifications
You must be signed in to change notification settings - Fork 245
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
[ refactor ] fixes #2568; proves full symmetry for Bijection
#2569
base: master
Are you sure you want to change the base?
Conversation
Bijection
Bijection
Bijection
@@ -20,16 +20,16 @@ | |||
module Function.Bundles where | |||
|
|||
open import Function.Base using (_∘_) | |||
open import Function.Consequences.Propositional | |||
using (strictlySurjective⇒surjective; strictlyInverseˡ⇒inverseˡ; strictlyInverseʳ⇒inverseʳ) |
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 is tempting to want to inline these, as, in the Propositional
case, they each take on a particularly simple form which is obscured by the three levels of indirection in these definitions...
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.
inline these 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.
Firstly, in Function.Consequences.Propositional
(see latest commit), but one could even imagine inlining these definitions (given how simple they are) at any call-site...?
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.
Last words on this:
- I think it is better, given that the types of the corresponding lemmas in
Function.Consequences.Propositional
differ from those inFunction.Consequences{.Setoid}
(by makingf
explicit), we can, moreover should, feel free to reimplement them, as is now done; - the explicit quantification seems necessary in the handful of call-sites where these lemmas are actually used (and with enough of them, that I no longer propose inlining them there)...
- ... BUT is wrong for
strictlyInverseʳ⇒inverseʳ
where it isf⁻¹
which should be supplied explicitly, and notf
! Fixing this should form part of [ DRY ] StreamlineFunction.*
hierarchy #2570
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.
Spectacular work. Just one question and one nitpick.
@@ -20,16 +20,16 @@ | |||
module Function.Bundles where | |||
|
|||
open import Function.Base using (_∘_) | |||
open import Function.Consequences.Propositional | |||
using (strictlySurjective⇒surjective; strictlyInverseˡ⇒inverseˡ; strictlyInverseʳ⇒inverseʳ) |
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.
inline these where?
src/Function/Consequences.agda
Outdated
@@ -23,22 +24,22 @@ private | |||
a b ℓ₁ ℓ₂ : Level | |||
A B : Set a | |||
≈₁ ≈₂ : Rel A ℓ₁ | |||
f f⁻¹ : A → B | |||
to f f⁻¹ : A → B |
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 understand that the way variables work, this declaration of f
and f⁻¹
will actually have the 'right' things in practice, I find the declaration here confusing, i.e. I would prefer an additional line with f⁻¹ : B → A
for clarity.
And I thought you'd rename f
to to
and f⁻¹
to from
everywhere?
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.
Re: variables.
You're absolutely right. Much clearer for ux/ergonomic reasons, and the typechecker needn't care!
Re: inlining.
Two things about this:
- I'd thought these lemmas weren't used quite as much as they are (I was really focused on their use in
Function.*
), so the case for inlining is less strong now - that said, the
Function.Consequences.Propositional
version probably should be inlined/redefined in that module, because in that setting, the waycong
andtrans
are defined for_≡_
means that they take a much simpler form than that inheriting fromSetoid
...
Re: f
and f⁻¹
.
I'd thought to have to
and from
for the new uses (and not simply those in Structures
/Bundles
), but I'm not really a fan of 'gratuitous' alpha-conversion elsewhere, if only for the sake of keeping the footprint of the PR smaller. But I could be persuaded... ;-)
src/Function/Consequences.agda
Outdated
@@ -81,6 +82,23 @@ strictlySurjective⇒surjective : Transitive ≈₂ → | |||
strictlySurjective⇒surjective trans cong surj x = | |||
Product.map₂ (λ fy≈x z≈y → trans (cong z≈y) fy≈x) (surj x) | |||
|
|||
module IsSurjective {f : A → B} (surjective : Surjective ≈₁ ≈₂ f) 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.
Named modules like this are usually an indication that they deserve their own top-level module? Do we really need a named module?
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.
Hmmm... the perennial debate between us about sub-modules!
Here, I would push back and say: yes, because when we instantiate that module in Function.Structures.{IsSurjection|IsBijection}
and Function.Bundles.{Surjection|Bijection}
, we precisely want to import a whole bunch of related definitions and properties; that is, as the comment says, this is a whole theory of the left inverse map from
.
That being said, I'm now more tempted to say that it should therefore be lifted out as a top-level module, but for the fact that it also is worth specialising this theory to Setoid
s in Function.Consequences.Setoid
, and I wasn't entirely clear what the linked module structure of those would look like:
Function.Consequences.Section.Base
for what is nowFunction.Consequences.Section
?Function.Consequences.Section.Setoid
for what is nowFunction.Consequences.Setoid.Section
?
each importing from the appropriate Function.Consequences
module? or simply Function.Section.{Base|Setoid}
instead?
Advice welcome!
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.
So... I've had time to sleep on this, and I'm coming round to your way of thinking, or at least, some way towards it:
- initially, I followed the path of
- start with properties of an arbitrary
Surjective f
relative to arbitrary 'equalities', inFunction.Consequences
- specialise to those equalities being derived from
Setoid
s, inFunction.Consequences.Setoid
import
those inFunction.Structures
- and in
Function.Bundles
- deploy in
Function.Construct.Symmetry
(Of course, there's a lot of post-hoc rationalisation involved here... but the layering of abstractions did seem at least congruent with other existing development paths instdlib
, so I'm not going to litigate this idealised design process further)
- start with properties of an arbitrary
To your question about separate top-level modules, I suppose we could now say: only the last three points of the list above matter, and the only real deployment of module Section
is in Function.Structures.{IsSurjection|IsBijection}
, EXCEPT for the deprecations of injective
, surjective
, bijective
in Function.Construct.Symmetry
. Grrrrr.
So a possible refactoring (modulo those deprecations, which seem to REQUIRE the definition of Function.Consequences.Section
, so perhaps could be further inlined in that deprecation block?) ahead of merging might be to lift all of Function.Consequences.Setoid.Section
directly into Function.Structures.{IsSurjection|IsBijection}
Is that more what you had in mind?
Summarising the discussion above:
Let me know how you see this going forward! UPDATED: given how much cleaner is the PR if we just go straight to the |
A comprehensive overhaul building on the proposed refactoring in #2568 . Finally finished, I think!
UPDATED: now all 'done', and a lot tighter/smoother/cleaner
Function.Consequences
? (check implicit/explicit bindings of lemmas there, esp. in their use later)Bijection
now proved, old weaker formsym-≡
etc. now deprecated (this was abug
!)Congruent ≈₂ ≈₁ section
outright under similar equational assumptions!Function.Construct.Symmetry
Congruent ≈₁ ≈₂ f
from{injective|bijective}
IsBijective
, and ofBijection
, without detour viaInverse
TODO:
CHANGELOG
Function.Construct.Symmetry
Function.Properties.Bijection
to prove full symmetry ofIsBijective
etc.NB.
Function.*
to rationalise the existence of a section to a givenSurjective f
#2568 are optional, it's more a case of agreeing the 'right' names; existing commits make the change so as to highlight where the work has happened*WithoutCongruence
constructions, but a v3.0 cleanup PR would remove the old proofs entirely and rename to remove that suffix