-
Notifications
You must be signed in to change notification settings - Fork 242
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
Port reverse lemmas to Data.Vec
(fixes #942)
#1668
Conversation
I renamed |
Data.Vector
(fixes #942)
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.
Awesome work! Added some thoughts 😄
Many thanks for the fine-toothed comb! Agree, there are lots of things which need improvement/reworking esp. wrt implicit/explicit arguments, so I'm grateful for your second look. I'll try to do that slowly over the next few days. And yes, there is a reason (those scary types!) why these lemmas were not |
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.
A few more (minor) comments
src/Data/Vec/Properties.agda
Outdated
map-const [] _ = refl | ||
map-const (_ ∷ xs) y = P.cong (y ∷_) (map-const xs y) | ||
|
||
map-++-commute : ∀ {m} {n} (f : A → B) (xs : Vec A m) (ys : Vec A n) → |
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.
In general I think we're not very consistent about whether we call such properties map-++
or map-++-commute
. I think the majority of properties go with the former, and it's also slightly shorter, and I haven't come across any places where the naming scheme breaks down, so I would probably go with the map-++
variety if we can?
Obviously if people have a different opinion happy to hear them!
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.
Fair enough. As with some of the other style comments above, I think I found myself attempting, not always faithfully, to emulate the naming conventions of Data.List.Properties
... and I confess I'd prefer short names if possible, but your remark about reverse-foldr
etc. had me wondering, in such a 'commute'/'complex=>simple' unfolding, which order even the commuting operations should be named. I had also wondered whether (see Data.Vec.Base
for example) whether reorganising this file so that everything was stated in terms of foldr
and foldl
might be a good idea, or a terrible one...
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. Originally, there was also a sum-++-commute
... another deprecation opportunity?
Edited: sum-++-commute
now deprecated in favour of sum-++
.
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.
Yes, in a perfect world we would be consistent about the ordering as well. I think lets hold off any reorganisation in terms of foldr
and foldl
for at least in this PR!
src/Data/Vec/Base.agda
Outdated
[] ∷ʳ y = [ y ] | ||
(x ∷ xs) ∷ʳ y = x ∷ (xs ∷ʳ y) | ||
{- better ? | ||
_∷ʳ_ {A = A} xs y = foldr ((Vec A) ∘ suc) (λ x r → x ∷ r) [ y ] xs |
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 think it's probably readable the way you have it at the moment 👍
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.
But see my comment above. BTW, _∷ʳ_
was already defined; it was the foldl
-based definition of _ʳ++_
which had me reconsidering those functions defined as foldr
s... towards some grand (folly of a) general foldl
/foldr
commutation result.
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 here's a strange thing: I was doing some more reorganising, to try to see if unfold-reverse
and unfold-ʳ++
could be rewritten in terms of foldr-reverse
(you'd think, right? at least in view of the above ;-)) and find myself in a situation where agda seems reluctant to unfold the recursion equations for foldl
correctly, even with every argument fully written out... starting to wonder if there is a bug?
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.
Or then again, it perhaps would require _∷ʳ_
to be written as a foldr
for that idea to work... instead of being, clause for clause, identical to a foldr
instance... hmmm.
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.
Above conjecture correct. So... functions defined by pattern-matching are not intensionally equal to (suitable instances of slightly more higher-order) functions with the same pattern-matching behaviour. Hmmm. Generativity bites again? Oh well.
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.
And... List.Properties
again leads the way with a bunch of *-is-foldr
lemmas. I'll leave those to another day perhaps
(For when you get back to |
OK, thanks @JacquesCarette for the tip... but you might need to make the hint more explicit. As it is, I end up using ʳ++-∷ : ∀ {m n} y (xs : Vec A m) {ys : Vec A n} →
(xs ʳ++ (y ∷ ys)) [ m + suc n ]≡ᵥ[ suc m + n ] ((y ∷ xs) ʳ++ ys) I take, reading between the lines, @MatthewDaggitt 's hint that much of what was in So if you can see a way to prove the above lemma without appealing to |
For your |
Thanks for all the changes @jamesmckinna. I've gone through and had a very minor tidy-up. The things I did do were:
Once the tests go green, I'll merge it in! |
@MatthewDaggitt Happy New Year! with many thanks for all the shepherding through last year of my various PRs, not least this particular beast. Here's to many more such... ;-) |
@JacquesCarette thanks for the suggestion regarding I think that my only (defensible?) reasons for introducing the new notation were:
All in all, the eventual reworking of Much food for thought, however, for which my thanks. But the mention of issue #568 above indicates that perhaps even more thought is required... |
And to you too 🎉
Will reply in the issue #1252 but long story short, I think I'm happy the names as they are now! |
Let's get this landed! |
@jamesmckinna I'm all for exploration. Especially around where people currently pull out Your instincts perfectly matched my previous thinking (on I think the main lesson (for me) is/was that none of the constructions around permutations actually care that the two indices are the same, but they do preserve that 'sameness'. Whereas in other settings, why they are the same matters. There are, after all, In other cases, such as vector operations, 'index shuffling' matters - so recording which permutation you've used may indeed matter. At least, that's my current thinking. [It's also an insight that was there in my work with Brent Yorgey in Species a long time ago... but we got referees who didn't like Species, so it never got published. One day, people will see that there's more to the world than just polynomials...] |
Thanks @JacquesCarette for the reinforcement of the lessons imparted in issue #1610... on which I (still) need to meditate. As indeed on Species... having failed to learn the lessons of Brent's "Oh My!" Pearl a few years back. Regarding Interesting to learn, albeit second-hand, the contrasting experiences of Concretely, I still have two open questions about the
Happy to take this discussion offline! |
Sorry I've taken so long to reply to this.
So everything should go to the most general place it can live. If it holds for arbitrary binary relations then it should go in the
Err I have to confess you can probably answer that one better than I can right now, as I haven't looked closely at these modules for a while. I suspect that you probably don't need it as much. |
No worries, it's a slightly stake thread for me atm, so this is a 'keepalive' as much to myself as anything: OK re 'most general' policy, but I haven't had the time to investigate how general the results are; and... duly noted regarding the syntax extensions, although are the 'usual' |
Yes, reasoning combinators are available over many arbitrary binary relations, see the |
Up to, but not including, those lemmas of Andreas which would require heterogeneous equality.
And the
CHANGELOG
...