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

Add Raw bundles to Relation.Binary.* hierarchy #2491

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jamesmckinna
Copy link
Contributor

@jamesmckinna jamesmckinna commented Oct 2, 2024

This addresses the Relation.Binary.Bundles.Raw part of #2274 ahead of any subsequent refactoring of the Algebra hierarchy in earnest, to exploit the standardisation of vocabulary/fixity envisaged there.

Adds:

  • RawX bundles systematically to mirror those in Relation.Binary.Bundles.X
  • rawX manifest fields to each of those in Relation.Binary.Bundles.X

NB. Possible issues:

@Taneb
Copy link
Member

Taneb commented Oct 2, 2024

This looks pretty good, but I do think that the changes to Relation.Binary.Bundles to export these should happen in this PR

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Oct 2, 2024

UPDATED: Please ignore this comment!

The latest round of debugging commits suggest that the Eq submodules of Reation.Binary.Bundles.X seem to exist solely to play the 'helper' role envisaged by @JacquesCarette in exporting an extended signature for Setoids with the negated equality symbol... so a 'natural' further refactoring would be to remove all this Eq indirection completely? (At the cost of reifying my difference of opinion with Jacques...).

I'm not clear about the history of these things, but why do we have the Eq submodules factored out in this way? UPDATED: oh, I see that it's one way to ensure that refl and trans as field names don't clash between the underlying isEquivalence and the corresponding IsPreorder properties. I wonder if that was the good/better/best design choice, in favour of having fields names ≲-refl, ≲-trans etc.? Qualified names... hmmm.

@jamesmckinna
Copy link
Contributor Author

This looks pretty good, but I do think that the changes to Relation.Binary.Bundles to export these should happen in this PR

Now added, and manifest at the top updated to reflect those additions.

@JacquesCarette
Copy link
Contributor

git blame on Relation.Binary.Bundles tells me the first appearance of such a module Eq dates from September 2019 by @MatthewDaggitt back when the file was Relation.Binary.Packages in commit 7e00c92.

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.

I'm somewhat undecided if this is the right thing to do. The code seems fine. It is definitely more consistent with what has been done elsewhere, so as a default, this is moving in a coherent direction. I'm just not quite ready to 'approve'.

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Oct 2, 2024

@JacquesCarette writes:

I'm somewhat undecided if this is the right thing to do. The code seems fine. It is definitely more consistent with what has been done elsewhere, so as a default, this is moving in a coherent direction. I'm just not quite ready to 'approve'.

Beyond your comments on the originating issue #2274 can you say (more) about why you are/might be hesitating? (UPDATED: have just seen your more recent comment there... interesting!)

@JacquesCarette
Copy link
Contributor

Teaching with PLFA right now, have updated to agda-2.7 and stdlib-2.2, and some things have gotten worse. I've got some issues to bring up when I have the time. One example: having Irrelevant show up in some of the goals when working on some stuff to do with Negation was unexpected and unwelcome. There's more.

@jamesmckinna
Copy link
Contributor Author

jamesmckinna commented Oct 2, 2024

Teaching with PLFA right now, have updated to agda-2.7 and stdlib-2.2, and some things have gotten worse. I've got some issues to bring up when I have the time. One example: having Irrelevant show up in some of the goals when working on some stuff to do with Negation was unexpected and unwelcome. There's more.

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2, or even on the PLFA tracker? hard to see how Relation.Nullary.Irrelevant might show up, except via Relation.Nullary.Negation.Core, other than... that should have happened before v2.0...? Shoutout also to @wenkokke who curates PLFA/agda/agda-stdlib compatibility...

@JacquesCarette
Copy link
Contributor

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2,

The example is of the latter kind. But the reason I'm hesitating is related to me finding teaching with post-v2.0 stdlib a bit awkward compared to 1.7.

@jamesmckinna
Copy link
Contributor Author

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2,

The example is of the latter kind. But the reason I'm hesitating is related to me finding teaching with post-v2.0 stdlib a bit awkward compared to 1.7.

Still: suggest you move this thread off this PR?

@wenkokke
Copy link
Contributor

wenkokke commented Oct 2, 2024

Are these issues to do with this PR, or general comments deserving a separate issue for v2.1/v2.2,

The example is of the latter kind. But the reason I'm hesitating is related to me finding teaching with post-v2.0 stdlib a bit awkward compared to 1.7.

Could you submit this as a separate issue, so that we can discuss these issues in detail?

@JacquesCarette
Copy link
Contributor

@wenkokke wrote:

Could you submit this as a separate issue, so that we can discuss these issues in detail?

planning to! I've been swamped, so this is still in my ToDo queue.

@MatthewDaggitt
Copy link
Contributor

I'm not clear about the history of these things, but why do we have the Eq submodules factored out in this way? UPDATED: oh, I see that it's one way to ensure that refl and trans as field names don't clash between the underlying isEquivalence and the corresponding IsPreorder properties. I wonder if that was the good/better/best design choice, in favour of having fields names ≲-refl, ≲-trans etc.? Qualified names... hmmm.

Yes, in retrospect it wasn't the best idea, but at the time, it was the only way to add them that preserved backwards compatibility.

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.

The other refactorings look good though!

-- RawPreorder
------------------------------------------------------------------------

record RawPreorder c ℓ₁ ℓ₂ : Set (suc (c ⊔ ℓ₁ ⊔ ℓ₂)) where
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so following the pattern of Algebra.Bundles.Raw, then bundle equivalence is determined not by the names of the relation, but by their signature. For example both CommutativeMonoid and Monoid both have a single RawMonoid base.

Therefore to be consistent I think RawPreorder, RawPartialOrder, RawStrictPartialOrder and RawApartness relation should all be a single record... With what name I don't know...

If the purpose of this is to introduce syntax, then I think we need to use some other mechanism other than Raw bundles...

Copy link
Contributor Author

@jamesmckinna jamesmckinna Oct 7, 2024

Choose a reason for hiding this comment

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

Two things:

  • good point about shared signatures (and _∼_ would be the 'canonical' choice of symbol here?), and then let the corresponding Rawsub bundles in Bundles handle the renaming to orthodox/rectified syntax?
  • as with discussions with @JacquesCarette , for me the issue is one of being able, for a given raw signature, to enrich it with 'obvious' derived operations which allow smoother treatment of subsequent properties etc. The 'syntax' aspect here is really about standardising notation for these otherwise useful/important derived notions...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Latest commit introduces RawRelation in Relation.Binary.Bundles.Raw and associated rawRelation fields in Relation.Binary.Bundles, together with all the relevant hiding/renaming directives to fix the syntax.

Is this an improvement?

@jamesmckinna
Copy link
Contributor Author

So, while I would still be happy/happier to see this merged (despite @JacquesCarette 's hesitation about doing so), it's somewhat ironic that we seem, via this route, to have reinstated the 'old' vocabulary (and then erased it via renaming... ;-)), cf. #2099 and its precursor issues/PRs. But I think that each time I/we go round this merry-go-round, things do actually improve?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants