Skip to content

Fix to the definitions of min and max#1270

Merged
mortberg merged 1 commit intoagda:masterfrom
LorenzoMolena:min-fix
Dec 31, 2025
Merged

Fix to the definitions of min and max#1270
mortberg merged 1 commit intoagda:masterfrom
LorenzoMolena:min-fix

Conversation

@LorenzoMolena
Copy link
Contributor

While working on a future PR, I noticed that when performing a "with-abstraction with UsingEq" on n <ᵇ m, each case was not normalizing to the expected term.
This issue is resolved by adding UsingEq into the definitions of min and max. Interestingly, all proofs that relied on "with-abstraction withot UsingEq" still works unchanged.

I added a comment about this limitation above the definition of UsingEq, but I'm not entirely sure how clear it is, so I also referenced its use in min and max over ℕ.
Please let me know if there is a clearer way to explain this behavior.

@mortberg
Copy link
Collaborator

Why not just define min and max by recursion without any with or calls to <?

@mortberg
Copy link
Collaborator

So having min (suc n) (suc m) = min n m and similarly for max

@anshwad10
Copy link
Contributor

Why not just define min and max by recursion without any with or calls to <?

They made it like this in #1262 for efficiency, because the _<ᵇ_ builtin is faster than recursing

@anshwad10
Copy link
Contributor

Also, I think this is precisely the advantage of using inspect over UsingEq, because when using inspect we abstract on the value of n <ᵇ m and the path separately

@mortberg
Copy link
Collaborator

Ah right, I had forgotten about that. I wonder how much one should worry about efficiency for functions on unary nat though... If we want things to be fast we should at least have a binary representation?

@anshwad10
Copy link
Contributor

Ah right, I had forgotten about that. I wonder how much one should worry about efficiency for functions on unary nat though... If we want things to be fast we should at least have a binary representation?

I don't know if that would help; It wouldn't be better in terms of storage at least, because Agda compiles Nat to built in arbitrary-precision binary integers, and idt it would speed up arithmetic because the builtin operations are O(1) while binary arithmetic is O(logN)

@marcinjangrzybowski
Copy link
Contributor

@mortberg this is part of work to enable fast operations on Integers -> Rationals -> and finaly beeing able to quickly calculatate things like Nth digit of e in #1182 :)
@LorenzoMolena identified bootlenecks, and we have very promising experiments.
We discussed those plans with @felixwellen , but are happy to meet @mortberg to get the feedback on our plan.
With @LorenzoMolena , we will be doing code sprint hopefully integrating this work, 2 weeks from now during AIM.

@LorenzoMolena
Copy link
Contributor Author

@mortberg I realized that I haven't actively participated in the discussion so far, which may have unintentionally given the impression that this PR is not yet ready to be merged.
The points raised by @anshwad10 and @marcinjangrzybowski are correct: I intentionally chose this definition based on a boolean comparison (which is linked to a builtin) in order to achieve better performance. I tested this approach, and it indeed results in an improvement.

Anyway, those changes have already been merged previously. In this PR, I am only adding an additional UsingEq to the definitions, in order to make some proofs easier to write. As an example of this can be found in this case distinctions in #1276.

In my view, the one remaining aspect that might need review is the new comment above the definition of UsingEq.
If that also sounds clear to you, then I believe this should be ready to merge.

@mortberg mortberg merged commit 16e6988 into agda:master Dec 31, 2025
1 check passed
LorenzoMolena added a commit to LorenzoMolena/cubical that referenced this pull request Jan 24, 2026
* Resolve naming inconsistency between Iso and Category.isIso (agda#1274)

s/leftInv/ret; s/rightInv/sec

---------

Co-authored-by: Madeleine Sydney Ślaga <msyds@deertopia.net>

* isIso name change fix (agda#1281)

* Update fin (agda#1279)

* Exchange Fin in Cubical.Data.Fin for the deifinition from Cubical.Data.Fin.Inductive. WIP

* Fixed finj, predFin, inject<, flast, ¬Fin0 to work with new definition of Fin.

* Minor changes

* Fix sumFinGen

* fix any?

* Everything type checks now. Important changes:
- Implicit argument added to toN
- Implicit arguments in toN-injective slightly changed
- Argument changed from implicit to explicit in elim

* some formatting

* change ¬Fin0

* remove comments

* Add <ᵗ-asym

* make properties type check

* Remove dependencies on Cubical.Data.Fin.Inductive.Base

* Things that work

* Properties type check

* more working files

* factorEquiv

* comment fix

* LehmerCode type checks

* SumFin.Base type checks

* some cleanup and Axiom.Choice type checks not reliant on old Fin

* Remove dependence on Fin.Inductive

* Remove dependence on Fin.Inductive

* Remove dependency on Fin.Infuctive

* Remove dependency on Fin.Inductive

* Remove dependencies on Fin.Inductive

* type check

* fix FinSet

* _<ᵗ_ is well founded

* type checks with new Fin

* Everything builds

* Move everything from Fin.Inductive.Properties to Fin.Properties and remove Fin.Inductive, Fin.Inductive.Base, Fin.Inductive.Properties

* some cleanup

* more cleanup

* fix trailing whitespaces

* files end with newline

---------

Co-authored-by: croos <76143n@gmail.com>

* fix defintions of `min` and `max`, add comment on limitations of `UsingEq` (agda#1270)

* Change implementation of Inductive Order on Nat (agda#1283)

* use boolean comparison in `Nat.Order.Inductive`, add non-strict and flipped inductive order, make faster instance search in `Fin.Literals`

* fix typo

* Place holder for paper (agda#1286)

* placeholder

* whitespae

* Cellpaper2 (agda#1287)

* placeholder

* whitespae

* form

* fix imports: `Fin.Inductive.Base/Properties` → `Fin.Base/Properties`

---------

Co-authored-by: Madeleine Sydney Ślaga <65362461+msyds@users.noreply.github.com>
Co-authored-by: Madeleine Sydney Ślaga <msyds@deertopia.net>
Co-authored-by: Max S. New <maxsnew@gmail.com>
Co-authored-by: Caroline Roos <croos.math@gmail.com>
Co-authored-by: croos <76143n@gmail.com>
Co-authored-by: Axel Ljungström <49276137+aljungstrom@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants