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

Simplify non-integer code #698

Closed
wants to merge 2 commits into from
Closed

Simplify non-integer code #698

wants to merge 2 commits into from

Conversation

csoroz
Copy link
Contributor

@csoroz csoroz commented Jul 30, 2019

I've used this test code (adapted from the C++ reference implementation) to check that the Golden test cases give the same results: https://gist.github.com/csoroz/fc337aad325417526f5b153e70502f28

@csoroz csoroz requested a review from mgudemann as a code owner July 30, 2019 14:50
-- A_{-1} = 1, A_0 = b_0
-- B_{-1} = 0, B_0 = 1
-- A_n = b_n*A_{n-1} + a_n*A_{n-2}
-- B_n = b_n*B_{n-1} + a_n*B_{n-2}
--
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've kept this hunk because the expressions are simpler ('n' vs 'n+1'), vertical alignment highlights its similarities and differences, is the same form in which are written in the doc, and also matches the parameters of the function implementation.

Copy link
Contributor

@mgudemann mgudemann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@mgudemann mgudemann left a comment

Choose a reason for hiding this comment

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

It seems the suggested change is needed

…ral.hs

Co-Authored-By: Matthias Güdemann <matthias.gudemann@iohk.io>
@csoroz
Copy link
Contributor Author

csoroz commented Aug 6, 2019

It seems the suggested change is needed

Hmm, I don't understand why is needed. Do you mean that the Show constraint is unnecessary here?
There are other cases where Show is not required. In fact the only functions that need the Show constraint are ln' and lncf because they show the argument in the error case, and (***) as it uses ln'.

@mgudemann
Copy link
Contributor

I got a compilation error with stack build for this redundant constraint, it didn't appear because of buildkite not running, I think

@csoroz
Copy link
Contributor Author

csoroz commented Aug 6, 2019

Oh, I see. It's due to the compiler flags -Wredundant-constraints and -Werror. I was compiling with -Wall only.

@mgudemann
Copy link
Contributor

mgudemann commented Aug 6, 2019 via email

edsko added a commit that referenced this pull request Jan 27, 2020
JaredCorduan pushed a commit that referenced this pull request Jan 27, 2020
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.

None yet

2 participants