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

[ refactor ] Possible refactoring of #2509 / #2513 ? #2517

Closed
jamesmckinna opened this issue Dec 11, 2024 · 5 comments · Fixed by #2524
Closed

[ refactor ] Possible refactoring of #2509 / #2513 ? #2517

jamesmckinna opened this issue Dec 11, 2024 · 5 comments · Fixed by #2524

Comments

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Dec 11, 2024

This issue may be regarded (and hence instantly dismissed) as a status:duplicate of #2509... but posted for 'documentation'/alternative implementation purposes.

I'm happy with the merged #2513 , fixing #2509 , and so too is the author of the OP. Thanks to @MatthewDaggitt for the speedy turnround time!

That said, a brain-worm has been worrying away at me as to whether a pattern-matching-free solution was available (slightly better abstraction wrt presentation/implementation of the Sublist relation...?), and it seems there is: relative to the existing solution in Data.List.Relation.Unary.All.Properties, we can refactor as follows

all⊆concat : (xss : List (List A)) → All (Sublist._⊆ (concat xss)) xss
all⊆concat {A = A} xss = All.tabulate xs∈xss⇒xs⊆concat[xss] where
  xs∈xss⇒xs⊆concat[xss] : xs ∈ xss → xs Sublist.⊆ concat xss
  xs∈xss⇒xs⊆concat[xss] {xs = xs} xs∈xss
    with prf ← Sublist.concat⁺ (Sublist.map Sublist.⊆-reflexive (Sublist.from∈ xs∈xss))
    rewrite List.++-identityʳ xs
    = prf

(and further simplification may be even be possible in the definition of prf itself? The ⊆-reflexive step is to pass from a rather simple relation via map to the more complex iterated Sublist (Sublist R) ... so this seems to have the flavour of the triangular identities for Sublist as a suitable 'monad' on relations... but I can't quite see the details yet)

This suggests that we might, instead, refactor by lifting out the above where-bound lemma into a self-standing one in Data.List.Relation.Binary.Sublist.Propositional.Properties (maybe even in Setoid? but then the proposed refactoring seems to stumble on the parameterisation on the Setoid, and the lemma shifts from A to List A... :-(, so perhaps the answer is to pull back all the way to Heterogeneous.Properties?) of the form

xs∈xss⇒xs⊆concat[xss] : xs ∈ xss  xs ⊆ concat xss
xs∈xss⇒xs⊆concat[xss] {xs = xs} xs∈xss
  with prf  concat⁺ (Sublist.map ⊆-reflexive (from∈ xs∈xss))
  rewrite List.++-identityʳ xs
  = prf

leaving the final implementation (under Data.List.Relation.Unary.All.Properties? or should this move as well?) simply as:

all⊆concat : (xss : List (List A))  All (Sublist._⊆ (concat xss)) xss
all⊆concat {A = A} xss = All.tabulate Sublist.xs∈xss⇒xs⊆concat[xss]
@MatthewDaggitt
Copy link
Contributor

If this can be done in a way compatible with the current module dependency graph then this seems like a nice refactoring.

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Dec 16, 2024

If this can be done in a way compatible with the current module dependency graph then this seems like a nice refactoring.

#2513 added

  • Data.List.Relation.Binary.Sublist.Propositional.
  • Data.List.Relation.Binary.Sublist.Propositional.Properties

to the dependencies of Data.List.Relation.Unary.All.Properties, but before a v2.2 release we're free to migrate the code for all⊆concat, aren't we?.

So the question is: do we want Data.List.Relation.Binary.Sublist.* to depend on Data.List.Relation.Unary.All.*, or vice versa?

As for the proposed refactoring, I still need to figure out the best way to achieve it, but that is, I think, independent of the above question?

@MatthewDaggitt
Copy link
Contributor

Data.List.Relation.Binary.Sublist.* to depend on Data.List.Relation.Unary.All.*

My feeling is definitely this way round. Binary relations should depend on unary relations.

@jamesmckinna
Copy link
Contributor Author

OK. I think we were perhaps too hasty with #2513 !

@jamesmckinna
Copy link
Contributor Author

Incoming PR #2524 refactors to put lemma under Sublist.Setoid.Properties, with (immediate) re-export to Sublist.Propositional.Properties.
I haven't managed to simplify the proof of the helper lemma, but that can follow later, if ever.

jamesmckinna added a commit to jamesmckinna/agda-stdlib that referenced this issue Dec 30, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 3, 2025
#2527)

* add: `⊆-Reasoning` on the model of `Subset`

* fix title comment

* add: `Heterogeneous.Properties.⊆-Reasoning`

* fixed `CHANGELOG`

* fixed `CHANGELOG`: added missing lemma name from #2517

* oops: fixed `import`

* reduce code duplication

* refactor: pick the correct `preorder`

* do not hide the useful reasoning combinators. Also the ≋-syntax needed a 4th argument.

* tidied up symmetry proof

* `fix-whitespace`

---------

Co-authored-by: Jacques Carette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants