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

Rational order #1071

Merged
merged 10 commits into from
Nov 2, 2023
Merged

Rational order #1071

merged 10 commits into from
Nov 2, 2023

Conversation

LuuBluum
Copy link
Contributor

Defines an ordering over the rationals and proves several key properties. Also refactors the rationals to use the integers instead of QuoInt; the original definition are relabeled as QuoQ and put into MoreRationals.

At some point, ideally under a different PR, QuoQ should be removed and all proofs involving it replaced with proofs against the variant using the default version of Int rather than QuoInt.

Copy link
Collaborator

@mortberg mortberg left a comment

Choose a reason for hiding this comment

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

Thanks, lots of good stuff in this PR!

A general question, can't the ring solver be used to prove a lot of the equations for Z that are needed in the proofs?

Cubical/Data/Rationals/Order.agda Outdated Show resolved Hide resolved
_#_ : ℚ → ℚ → Type₀
m # n = (m < n) ⊎ (n < m)

data Trichotomy (m n : ℚ) : Type₀ where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't have this for nat and int already? Should be upstreamed

Copy link
Contributor Author

@LuuBluum LuuBluum Oct 30, 2023

Choose a reason for hiding this comment

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

We do, but the issue here is that it only works for [ a , b ] [ c , d ] cases. Upstreaming it would require a gnarly pattern match like the definition of < or <=.

Cubical/Data/Int/Properties.agda Show resolved Hide resolved
Cubical/Data/Int/Properties.agda Show resolved Hide resolved
Cubical/Data/Int/Properties.agda Show resolved Hide resolved
@mortberg
Copy link
Collaborator

Can someone who knows how the CI really works help with fixing:

agda: Heap exhausted;
agda: Current maximum heap size is 3758096384 bytes (3584 MB).
agda: Use `+RTS -M<size>' to increase it.
make: *** [GNUmakefile:53: check] Error 251
Error: Process completed with exit code 2.

Maybe @felixwellen or @mzeuner ?

@LuuBluum
Copy link
Contributor Author

The heap issue seems to be happening on master as well, so it's a broader issue with the library.

@felixwellen
Copy link
Collaborator

Please rebase/merge master

@mortberg
Copy link
Collaborator

mortberg commented Nov 2, 2023

Thanks for this! Looking forward to future PRs cleaning things even more

@mortberg mortberg merged commit c703c36 into agda:master Nov 2, 2023
1 check passed
@LuuBluum LuuBluum deleted the RationalOrder branch November 2, 2023 15:50
@MatthiasHu
Copy link
Contributor

@LuuBluum Could you explain the motivation for 1bc03c7 ? The previous definition of rec2 was shorter and seems more readable and elegant to me.

@LuuBluum
Copy link
Contributor Author

@LuuBluum Could you explain the motivation for 1bc03c7 ? The previous definition of rec2 was shorter and seems more readable and elegant to me.

Pattern matching. Defining rec2 recursively (as it was before) results in significant slowdown on usage due to needing to carry over the proof of transitivity over the quotient relation every time you invoke it, particularly with things that don't care about that proof at all.

For context, under the previous definition of rec2, if I were to define the operations on the rational numbers that way, you would never be able to use them because they could never compute. It was just prohibitively slow. This was the only way to actually define the rationals as a set quotient so that it could actually be used.

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