-
Notifications
You must be signed in to change notification settings - Fork 257
Add cartesianProductWith⁻
and cartesianProduct⁻
to All
#2824
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
base: master
Are you sure you want to change the base?
Add cartesianProductWith⁻
and cartesianProduct⁻
to All
#2824
Conversation
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 nitpicks.
* In `Data.List.Relation.Unary.All.Properties`: | ||
```agda | ||
cartesianProductWith⁻ : ∀ f → | ||
f Preserves₂ _≈₁_ ⟶ _≈₂_ ⟶ _≡_ → |
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.
There is no point right-indenting so much, go ahead and only indent the type by 2-4 spaces, so as to use fewer lines
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.
Also: no need for the explicit quantifiers for f
and xs
/ys
in this type?
(But there seems to be no explicitly documented consensus on how much detail of such kind we expect in a CHANGELOG
entry...?)
Indeed, are you sure that xs
/ys
even need to be explicitly quantified at all here?
All P (cartesianProductWith f xs ys) → | ||
(∀ {x y} → x ∈₁ xs → y ∈₂ ys → P (f x y)) | ||
cartesianProductWith⁻ {P = P} f pres (x′ ∷ xs) ys Ps {y = y} x∈x′∷xs y∈ys | ||
with x∈x′∷xs | ++⁻ (map (f x′) ys) Ps |
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 would be nicer the use plain pattern-matching for the x∈x′∷xs
argument and then dispatch on with
.
Also: is there a way to use equational reasoning instead of rewrite
? This proof is far from limpid.
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 rather than use with
, as the result of that computation has a single-constructor type, it can be irrefutably matched with a pattern-matching let
instead, or even the new using
syntax?
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, if the issue is one of laziness, namely that different projections are used in each branch, then better to use proj₁
/proj₂
instead?
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.
Also: in each case branch, pointless eta-expansion wrt the y∈ys
argument...
The UPDATED: I think that the 'real' I think that this is the 'usual' problem with |
This PR adds
cartesianProductWith⁻
andcartesianProduct⁻
toData.List.Relation.Unary.All.Properties
.