Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
a recursive view of
Fin n
#1923a recursive view of
Fin n
#1923Changes from 36 commits
75fa9d0
629b415
c6a9994
d6e7b9c
d2173fb
6e0c4be
1021deb
a870d69
c766cc7
f43a21f
66bc0c7
8eff5d6
6576fe5
c18173c
d4069e9
6d4cc58
0e6db5e
fa28f4c
c51dd63
904fdbe
f3550ad
9a0d6ad
1dd382e
f10cb3f
d878b2c
c0d5651
2c94427
9bce07e
9012225
c7e7c9e
1f1f9f3
a88d39c
c962e14
6ba18be
c7b3acf
eae3f83
4a4a4e2
c8451d7
6d8979d
21500e9
c5ed38f
cb3eb95
7207c6e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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, my overall comment is that I'm not sure this is really a good match for instance search. Can you explain what the advantages of using instance search here are over simply applying the proofs manually? And why they're worth the decrease in the ability of the reader to comprehend the proofs?
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.
See above: not sure that I can! UPDATED: but now see below?
The original rationale (sic) for the use of instances was again, to try to workaround (based on what I observed from the error messages ;-) the limitations of the typechecker arising from working
--without-K
, viz. the inability to unify type indices involving non-constructor-but-nevertheless-injective defined symbols (an important bug/feature of the otherwise ostensible use of 'green slime' inView
types), and before I had even first heard ofinjectivity
hints inlossy-unification
(which I still don't quite understand how to use/exploit in such situations).The
View
type constructors here mention 'green slime' in their indices, but nevertheless (asview
witnesses!), define a provably-disjoint-and-total covering of the telescope of indices in the type... my deployment ofinstance
s was an idiomatic/idiosyncratic attempt to negotiate turning that 'provably-disjoint-and-total covering' into something the typechecker knows and can exploit.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.
Against that, I think that the example
inject₁⁻¹
of invertinginject₁
inREADME.DATA.Fin.Relation.Unary.Top
does mostly shine... only the examplereally fails in that regard?
To be more explicit: when we have a function with a partial inverse, long experience tells me it's actually a pain to have to explicitly supply a witness to its domain condition... you could say that I partly started on this development (besides wanting/needing the view for the Binomial Theorem) out of sheer frustration at the amount of wasted proof energy on checking the domain condition for
lower₁
, and the additional not-needed patterns involved in its definition, when the alternative, shown here, shows how the judicious use of views streamlines such definition/reasoning.Mind you, it seems as though I perhaps have not been as successful in advocating for, or winning, this point, as I had first thought.
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.
See most recent commits: I think I can now get more value out of (automatic) instance inference... but maybe that shows I still have much to learn about that! Specifically,
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: your original question
I think it is now the case that the proofs of the properties of the inverse function now show off the use of instance inference as (originally) intended. I suppose your question might remain as to whether, in practice, a hypothetical user of these example programs might have trouble producing witnesses when they want to appeal to the inverse function. But actually, I don't think that would be the case, precisely because the ambient proof context should contain enough witnessing information (eg in the scope of a
with
based on a call to theview
, or else through other explicit mention ofinject₁
in the types of things) to know that it is safe to appeal to the inverse function without further ado?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 for the explanation. I follow your reasoning about why the view pattern is useful and strongly agree with it! However, I've downloaded the code and had a play with it. Unfortunately fundamentally I don't think the remaining instances are doing very much. All the places in
README. .... .Top
where you actually end up instantiating the instances, the required form holds definitionally.You can actually see this in the way the instances are constructed, e.g. the only way to Agda can use instances search to find a proof of type
IsInject₁ i
is to havei
actually be definitionally equal toinject₁ v
for somev
in the first place.Furthermore, comparing the code in the README file for the properties
inject₁⁻¹
vs the properties oflower₁
inData.Fin.Properties
, the latter are far more readable in my opinion.Conclusion
I really like the view pattern, its neat and really useful, as witnessed by the nice proofs of
opposite
in the README file. However, I'm still of the opinion that the contents ofTop.Instance
is over-complicating things for minimal gain.In the interests of getting the view merged in, I therefore propose that in this PR we remove
Top.Instance
and the inverting inject code in the README file, and then merge as is.If you're still keen to push for the instance-based code, we can then have a in-depth follow up discussion (in a less asynchronous setting) about a second follow-up PR?