Skip to content

Conversation

carlostome
Copy link
Contributor

This PR adds proofs that projections from the product type are structure preserving maps (morphisms) from the direct product construction.

This not complete whatsoever, but at least it gives an initial template that can be extended over time.

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.

looks good to me

@jamesmckinna
Copy link
Contributor

jamesmckinna commented May 21, 2025

Hmm: some things to maybe (re) consider

  • names?
  • tighter imports
  • module organisation, including the inlining of the IsRelHomomorphism definitions, seems sub-optimal (see below)
  • the interesting morphism, namely into the product object, isn't defined, not a deal breaker but defining it might expose further some of the issues above?

Suggested refactoring:

  • add Relation.Binary.Morphism.Construct.Product first (to handle the IsRel* substrate, and factor out the dependency on the 'bare' Data.Product.Base structure) (PR incoming unless you pick this up) UPDATED: see [ add ] product structure on RawSetoid #2720
  • slice the module structure you have orthogonally: so that eg. a module Proj₁ (refl : Reflexive M._≈_) where defines the various versions of proj₁ for each of the algebraic categories... etc. (WIP)

Copy link
Contributor

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

And I also agree with @jamesmckinna's suggestion that the actual morphism should be listed explicitly.

@jamesmckinna jamesmckinna added status: blocked-by-issue Progress on this issue or PR is blocked by another issue. addition labels May 26, 2025
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.

  • level polymorphism
  • suggest refactor in the light of #2720

@carlostome carlostome force-pushed the carlos/add-projection-morphisms branch from 38c20f8 to 1b7d503 Compare June 5, 2025 12:32
@carlostome
Copy link
Contributor Author

carlostome commented Jun 5, 2025

I've tried to address the comments above.

However, I can't understand how the suggestion in:

  • slice the module structure you have orthogonally: so that eg. a module Proj₁ (refl : Reflexive M._≈_) where defines the various versions of proj₁ for each of the algebraic categories... etc. (WIP)

would work. By this I mean, that the argument (refl : Reflexive M._≈_) already depends on a chosen algebraic structure
M, and thus it does not make sense to define the "various versions" of functions under such module.

I already rebased this branch on top of (a squashed version of) jamesmckinna:rel-product (see #2720) to have access to "relational homomorphisms".

@jamesmckinna jamesmckinna removed the status: blocked-by-issue Progress on this issue or PR is blocked by another issue. label Jun 5, 2025
@jamesmckinna
Copy link
Contributor

jamesmckinna commented Jun 5, 2025

Thanks @carlostome yes I think you're right that my suggestion about restructuring the module organisation was wrong.
That said, I'd be tempted to replay the 'export' trick from Relation.Binary.Morphism.Construct.Product to make your definitions in Algebra.Morphism.Construct.DirectProduct a bit less unwieldy, both to define, and to deploy?

@jamesmckinna jamesmckinna added this to the v2.3 milestone Jun 25, 2025
@jamesmckinna
Copy link
Contributor

Now that #2720 has been merged, suggest we try to tie this one up for v2.3 also?

@carlostome carlostome force-pushed the carlos/add-projection-morphisms branch from 0b14e5b to 8ea743c Compare June 25, 2025 15:13
@carlostome carlostome requested a review from jamesmckinna June 25, 2025 15:17
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.

Apart form the final nitpick, looks good to go.
Many thanks for the contribution, and your patience during the review process!

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.

Yup, all looks good to me now!

@JacquesCarette
Copy link
Contributor

So we need an approve from @MatthewDaggitt .

@MatthewDaggitt MatthewDaggitt added this pull request to the merge queue Jul 2, 2025
Merged via the queue into agda:master with commit ebe659c Jul 2, 2025
12 checks passed
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.

4 participants