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

Negation on real numbers #1246

Merged
merged 25 commits into from
Feb 4, 2025
Merged

Negation on real numbers #1246

merged 25 commits into from
Feb 4, 2025

Conversation

lowasser
Copy link
Contributor

No description provided.

@EgbertRijke
Copy link
Collaborator

Awesome!

@EgbertRijke
Copy link
Collaborator

Some formatting needs to be edited, I'll give you a more detailed review later.

@lowasser lowasser changed the title Define the negation of a real number. Negation on real numbers Jan 31, 2025
@lowasser
Copy link
Contributor Author

I've attempted to update the formatting, PR title, etc. in accordance with what you've said elsewhere.

@EgbertRijke
Copy link
Collaborator

Your code looks very good! Thank you so much for all your contributions!!

Copy link
Collaborator

@fredrik-bakke fredrik-bakke left a comment

Choose a reason for hiding this comment

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

Very nice pull request!

src/real-numbers/negation-real-numbers.lagda.md Outdated Show resolved Hide resolved
src/real-numbers/negation-real-numbers.lagda.md Outdated Show resolved Hide resolved
src/real-numbers/negation-real-numbers.lagda.md Outdated Show resolved Hide resolved
@fredrik-bakke
Copy link
Collaborator

ah, whoops, sorry for the double up review

Co-authored-by: Fredrik Bakke <[email protected]>
Co-authored-by: Egbert Rijke <[email protected]>
@EgbertRijke
Copy link
Collaborator

It looks like this PR just needs a make pre-commit and it has a conflict with a previous change that needs to be addressed. To do this, update your branch:

  • git pull to get any changes made in the PR here on GitHub
  • git pull origin master to get the changes from master. This will give merge conflicts.
  • Resolve the merge conflicts
  • Run make pre-commit

@EgbertRijke
Copy link
Collaborator

@lowasser, would you mind wrapping up this pull request when you have time?

@lowasser
Copy link
Contributor Author

lowasser commented Feb 4, 2025

Yes, it's on my list.

@fredrik-bakke
Copy link
Collaborator

@EgbertRijke

Is there something wrong with our implementation of the real numbers, that we need lossy unification so much?

Now that you mention it, it likely boils down to the fact that our implementation of the rationals has to reduce fractions between every operation. Alternatively, we could follow 1lab's approach and switch to the definition of the rationals as a quotient rather than as the subtype of reduced fractions, that would probably fix this issue.

@EgbertRijke
Copy link
Collaborator

@EgbertRijke

Is there something wrong with our implementation of the real numbers, that we need lossy unification so much?

Now that you mention it, it likely boils down to the fact that our implementation of the rationals has to reduce fractions between every operation. Alternatively, we could follow 1lab's approach and switch to the definition of the rationals as a quotient rather than as the subtype of reduced fractions, that would probably fix this issue.

Could that really be the issue?

There's also a data definition of the rationals incoming in #1211, although I haven't done too much with that definition yet. But since it's data, it's probably fast.

@EgbertRijke EgbertRijke merged commit 0b11fe7 into UniMath:master Feb 4, 2025
4 checks passed
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