Skip to content
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

More list properties about catMaybes/mapMaybe #2389

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

omelkonian
Copy link
Contributor

src/Data/List/Properties.agda Outdated Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
@omelkonian
Copy link
Contributor Author

@JacquesCarette To sum up, let's agree that there are both pros and cons to using old-school eliminators in contrast to immediately using (dependent) pattern matching:

  • PRO: reusability! e.g. if we have some lemmas about maybe we could reuse them for deriving other proofs
  • CONS:
    • type inference (e.g. explicitly giving the motives in the examples above)
    • someone wanting to prove something about these definitions can immediately see what they have to do case analysis on, while with maybe that have to perform one additional mental step

Given that there is only one lemma about maybe 😞 (maybe-map) and that the inductive hypotheses in the proofs above can easily be shared using with IH ← ..., I have not followed your suggestion to use eliminators and stuck with matching on the maybes and so forth.

Feel free to push another commit on top of my branch to convert to eliminator-style proofs though...

@JacquesCarette
Copy link
Contributor

@omelkonian I'll comment on various of the details a bit later, but long story short: you make a very convincing case. I was overly optimistic.

Copy link
Contributor

@jamesmckinna jamesmckinna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great additions, but need to fix up layout and some of the with-based proofs in favour of the (simpler!) originals, I think.

Comment on lines 755 to 749
Any-catMaybes⁺ : ∀ {P : Pred A ℓ} {xs : List (Maybe A)}
→ Any (MAny P) xs → Any P (catMaybes xs)
Any-catMaybes⁺ {xs = nothing ∷ xs} (there x∈) = Any-catMaybes⁺ x∈
Any-catMaybes⁺ {xs = just x ∷ xs} (here (just px)) = here px
Any-catMaybes⁺ {xs = just x ∷ xs} (there x∈) = there $ Any-catMaybes⁺ x∈
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why here rather than added after L150 in the original?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for the sake of juxtaposition, I'd swap L758 and L759, as well as suppressing x and xs:

Suggested change
Any-catMaybes⁺ : {P : Pred A ℓ} {xs : List (Maybe A)}
Any (MAny P) xs Any P (catMaybes xs)
Any-catMaybes⁺ {xs = nothing ∷ xs} (there x∈) = Any-catMaybes⁺ x∈
Any-catMaybes⁺ {xs = just xxs} (here (just px)) = here px
Any-catMaybes⁺ {xs = just xxs} (there x∈) = there $ Any-catMaybes⁺ x∈
Any-catMaybes⁺ : {P : Pred A ℓ} {xs : List (Maybe A)}
Any (MAny P) xs Any P (catMaybes xs)
Any-catMaybes⁺ {xs = nothing ∷ _} (there x∈) = Any-catMaybes⁺ x∈
Any-catMaybes⁺ {xs = just __} (there x∈) = there $ Any-catMaybes⁺ x∈
Any-catMaybes⁺ {xs = just __} (here (just px)) = here px

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why here rather than added after L150 in the original?

Grouped the properties of catMaybe with the ones for mapMaybe, as it seemed more intuitive.
Also, lemma catMaybes-concatMap mentions concatMap which comes just before in the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, perhaps there is no 'right' order for module contents, so the rule-of-thumb ends up being: try, where possible, to follow in Properties the order in Base... but even that seems hard to stick to.

CHANGELOG.md Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
@JacquesCarette
Copy link
Contributor

As I agree with @jamesmckinna 's comments, I won't approve this PR (yet), even though I'm not happy with the parts that I had commented on myself.

@omelkonian omelkonian force-pushed the orestis/mapMaybe-catMaybe branch 2 times, most recently from 8d831bf to 8737650 Compare May 21, 2024 14:26
@jamesmckinna
Copy link
Contributor

Thanks for the continued updates, but I won't re-review until you signal it's time to do so though!

@omelkonian
Copy link
Contributor Author

@jamesmckinna ready for review

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff.

CHANGELOG.md Outdated Show resolved Hide resolved
src/Data/List/Properties.agda Outdated Show resolved Hide resolved
Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@omelkonian
Copy link
Contributor Author

@JacquesCarette @jamesmckinna Ready to merge?

Copy link
Contributor

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes from me.

Copy link
Contributor

@jamesmckinna jamesmckinna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the new with-based code (and the accompanying code motion) is an improvement over the existing pattern-matching based definitions, but you have two approvals from others, so I'm not going to stand in the way.

@jamesmckinna jamesmckinna dismissed their stale review June 3, 2024 09:25

Requested changes haven't been made, and don't seem likely to be anytime soon.

@omelkonian
Copy link
Contributor Author

omelkonian commented Jun 3, 2024

I don't think the new with-based code (and the accompanying code motion) is an improvement over the existing pattern-matching based definitions, but you have two approvals from others, so I'm not going to stand in the way.

Isn't the reuse of the inductive hypothesis (ih) clearly an improvement?

[sorry, hit edit instead of quote reply]

@jamesmckinna
Copy link
Contributor

I don't think the new with-based code (and the accompanying code motion) is an improvement over the existing pattern-matching based definitions, but you have two approvals from others, so I'm not going to stand in the way.

Isn't the reuse of the inductive hypothesis (ih) clearly an improvement?

In this case, I (personally) actually think no, it's not, at least not enough to justify the rewriting of existing code. Funnily enough, I think that both you and @JacquesCarette have convinced me, in different ways, and from different starting points, to view such code style with... scepticism for the time being. And one reason I dismissed my review was to avoid going back over this ground...so: I stand aside, as previously.

@andreasabel
Copy link
Member

Mmh, I tend to agree with James @jamesmckinna that with should be avoided where possible. The reason is that it internally generates a helper function which Agda might sometimes leak in printing. Also, it bears some hazards in proving things about the parent function as the name of the auxiliary function is not available to the user.
So, I think the general thrust should be to get rid of with where possible rather than introducing more withs.

@JacquesCarette
Copy link
Contributor

While I very much agree with @andreasabel 's general stance, especially for any definition that computes, I've gotten more relaxed about "pure proofs".

Really what's we're sorely missing (and this is, I believe, what @omelkonian was using it for) is a proper pattern-matching let that doesn't unfold eagerly. This is a real wart in the current Agda language (along with the same issue wrt module parameters).

@omelkonian
Copy link
Contributor Author

While I very much agree with @andreasabel 's general stance, especially for any definition that computes, I've gotten more relaxed about "pure proofs".

See my comment above on "meta-properties", which nullifies the distinction between programs and "pure proofs".

Really what's we're sorely missing (and this is, I believe, what @omelkonian was using it for) is a proper pattern-matching let that doesn't unfold eagerly. This is a real wart in the current Agda language (along with the same issue wrt module parameters).

Isn't the upcoming using syntax that I mentioned above already what you wish for (although I'm not sure about its unfolding behaviour, but the opaque feature should cover that).

@JacquesCarette
Copy link
Contributor

Yes, meta-properties, being proofs-about-proofs, do blur things. I've hated doing such proofs so much (in MLTT) that I block them out; when done 'properly', a la category theory where one adds higher-level cells, then these become sane again. subst hell is really the worst hell of them all.

I don't understand (yet!) enough of the details of using to know if it really going to be 'the' feature needed. I suspect that as long as there isn't some kind of let-like construct in the internal AST, it won't be, it'll merely be pleasant syntactic sugar.

@jamesmckinna
Copy link
Contributor

Shall we close the discussion and merge?

@andreasabel
Copy link
Member

andreasabel commented Jun 5, 2024

@omelkonian: Thanks, Orestis, for your patience and willingness to revert changes!
(with is gone again)

@jamesmckinna
Copy link
Contributor

@omelkonian: Thanks, Orestis, for your patience and willingness to revert changes! (with is gone again)

Seconded!

@jamesmckinna jamesmckinna added this pull request to the merge queue Jun 5, 2024
Merged via the queue into master with commit 17c84f4 Jun 5, 2024
2 checks passed
@omelkonian omelkonian deleted the orestis/mapMaybe-catMaybe branch June 5, 2024 14:32
andreasabel pushed a commit that referenced this pull request Jul 10, 2024
* More list properties about `catMaybes/mapMaybe`

- Follow-up to PR #2361
- Ported from my [formal-prelude](https://github.com/omelkonian/formal-prelude/tree/c10fe94876a7378088f2e4cf68d9b18858dcc256)

* Revert irrefutable `with`s for inductive hypotheses.
@andreasabel andreasabel mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants