Skip to content

Add/fix RightInverse for dependent products #2706

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

Merged
merged 7 commits into from
Apr 18, 2025

Conversation

cspollard
Copy link
Contributor

@cspollard cspollard commented Apr 17, 2025

This PR contains two changes:

  • adds a dependent product right inverse Σ-↪, which is dual to Σ-↩
  • renames left-inverse to right-inverse at src/Data/Product/Function/Dependent/Setoid.agda: as far as I can tell, this was a typo.

If there's interest, I could try to add a left-inverse dual to right-inverse in a similar way to what has been done for PropositionalEquality.

@jamesmckinna points out that this may be in conflict with #2583 . Since the PR here is rather small, it probably could be added after that one is merged -- I didn't look at what would be required.

(also: will make changes to ChangeLog if it's agreed this should be merged.)

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 great to me - but needs a CHANGELOG entry.

@jamesmckinna
Copy link
Contributor

@cspollard this looks fine to me, modulo:

So I think a slightly more conservative, but backwards compatible (and hence mergeable now), version, would be to reinsert left-inverse as a deprecated name (and... defined equal to right-inverse), and then in v3.0 we'll fix things up by removing that deprecation (even that goes beyond our usual practice, but I think we're planning to make so many wholesale changes in v3.0 that it won't matter in the great scheme of things).

Separately, and as a different solution to the problem, but one moreover future-proofed for your other suggestion about a 'real' left-inverse dual to right-inverse, would be to change the names entirely:

  • your new right-inverse (old: left-inverse) becomes rightInverse
  • left-inverse is deprecated as an alias of rightInverse
  • leftInverse becomes a name free to define now as part of this PR, or downstream, without any clash with the deprecated name.

The rationale for this is that for all of our other CamelCase record type/bundle names, we tend to observe the heuristic that concrete (typically: 'module-locally-canonical') instances should be named camelCase : CamelCase... and the usage left-inverse/right-inverse here seems a wart on such a naming scheme, which is otherwise observed everywhere else in Data.Product.Function.Dependent.Setoid!

The only contra argument to the above would be if we could somehow weaken the restriction to PropositionalEquality in the first component, in the spirit of the generalisations in #2583 but even if we could/can, those would similarly be an API change we could fold into a v3.0 breaking PR.

@jamesmckinna
Copy link
Contributor

Regarding the possible conflict:

  • sorry for having introduced that noise in the discussion on Zulip without thinking things through
  • the imports you make in Data.Product.Function.Dependent.Propositional from Function.Construct.Symmetry in any case do not touch those lemmas affected by [ refactor ] Symmetry of Bijection as a consequence of properties of a given Surjective function #2583 , so I think there is no actual conflict
  • the separate question of whether the constructions can all be generalised to non-propositional first components is a much larger one, and out-of-scope for this PR

@cspollard
Copy link
Contributor Author

Hi @jamesmckinna @JacquesCarette ,

Thanks for the careful review. Here's what I have done:

  • used the names rightInverse and leftInverse (which was easy enough to provide), as suggested by @jamesmckinna
  • deprecated left-inverse, as suggested by @jamesmckinna
  • added a note to CHANGELOG.md, as requested by @JacquesCarette

Please let me know if you see any other worthwhile changes!

I leave other updates for a future PR.

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.

CHANGELOG entries need fixing, and one or two other nitpicks, but otherwise looks great!

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.

One last typo, but otherwise looks great! Thanks!

@jamesmckinna
Copy link
Contributor

@JacquesCarette I'll set it to merge when ready, so once the typo is fixed, and you approve, we're good to go?

@jamesmckinna jamesmckinna enabled auto-merge April 18, 2025 12:43
@jamesmckinna jamesmckinna added this pull request to the merge queue Apr 18, 2025
Merged via the queue into agda:master with commit 8b8946a Apr 18, 2025
2 checks passed
@jamesmckinna
Copy link
Contributor

Ugh! I was too hasty in setting to merge when ready before the typo got fixed... we'll have to 'fix it up in the edit` ahead of any v2.3 release.

@cspollard
Copy link
Contributor Author

Hi @jamesmckinna sorry I missed it: what was the last typo?

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.

3 participants