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 functions for rounding rationals to integers #1585

Merged
merged 12 commits into from
Sep 21, 2021

Conversation

barrettj12
Copy link
Contributor

Implements functions for rounding rationals to integers, thereby solving #1063.

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.

Thanks for the great PR! I've added a few minor comments about library style, feel free to check out the style guide for more detail.

On the more major questions side, these don't have any particularly fancy import requirements so I think they could probably go in Data.Rational.Base?

It'd also be good to add their counter-parts to Data.Rational.Unnormalised while you're here? As an aside that might help you prove some properties around them as well, as the standard proof style is first to prove properties of the unnormalised operations, then prove they're isomorphic to the normalised versions and then transfer the proofs across.

@barrettj12 barrettj12 changed the title Add new module "Data.Rational.Rounding" Add functions for rounding rationals to integers Sep 7, 2021
@gallais
Copy link
Member

gallais commented Sep 8, 2021

Can we add some test cases to https://github.com/agda/agda-stdlib/tree/master/tests/data
to make sure we're getting the appropriate behaviour on edge cases?

@barrettj12
Copy link
Contributor Author

Can we add some test cases to https://github.com/agda/agda-stdlib/tree/master/tests/data
to make sure we're getting the appropriate behaviour on edge cases?

Sure, that sounds like a good idea. Is there a guide on how to do this? I don't have any experience with that.

@gallais
Copy link
Member

gallais commented Sep 8, 2021

Not really: it's a fairly new addition to the stdlib.
The easiest way is probably to:

  1. cp -R an existing directory
  2. Edit the main agda file to add your tests [1]
  3. Add the new test case to the the appropriate pool in the test runner
  4. Run the test suite and accept the update expected file
  5. Commit all of that

[1] They get run using the run file in the directory so you could have unit tests
of the form

_ : a \== b
_ = refl

and run would just make sure the file typechecks or you could have full blown
programs that get compiled & run. As you want.

@barrettj12
Copy link
Contributor Author

Side note: what should the fractional part of -3.7 be? I reckon 0.7.

@JacquesCarette
Copy link
Contributor

I'm sure there are coherent ways of making it 0.7, -0.7, 0.3 or -0.3. To me, it all depends on what relations with ceil, floor and/or round and addition you want to hold.

@barrettj12
Copy link
Contributor Author

Yeah, there doesn't seem to be a convention, so I will redefine round and fracPart to get symmetry, i.e.

round p = round (- p)
fracPart p = fracPart (- p)

@barrettj12
Copy link
Contributor Author

@gallais I've put in the new test cases under tests/data/rational. I think I've done it right.
@MatthewDaggitt I've implemented the changes you suggested, except I still haven't been able to prove anything useful about these functions. I'm happy to open a new issue for proving the properties.

@gallais
Copy link
Member

gallais commented Sep 8, 2021

Looks like you happen to have a trailing newline in your expected file.

@barrettj12
Copy link
Contributor Author

barrettj12 commented Sep 9, 2021

Looks like you happen to have a trailing newline in your expected file.

No, it appears that I didn't have a trailing newline, but now I've added one to match the other expected files. Now the tests are passing fine, but it doesn't want to play nice with GHC. The error was

Cannot add PPA: 'ppa:~hvr/ubuntu/ghc'.
ERROR: '~hvr' user or team does not exist.

which might have been caused by a faulty internet connection. Can we re-run those checks?

EDIT: The checks all look fine now.

@MatthewDaggitt
Copy link
Contributor

Looks good! Thanks for the PR!

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

Successfully merging this pull request may close these issues.

Add rounding operations to Data.Rational
4 participants