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

checkEuclideanRing #9

Open
jamesdbrock opened this issue May 23, 2023 · 3 comments · Fixed by #18
Open

checkEuclideanRing #9

jamesdbrock opened this issue May 23, 2023 · 3 comments · Fixed by #18

Comments

@jamesdbrock
Copy link
Member

Do we know why the checkEuclideanRing can't pass?

-- Data.checkEuclideanRing prxBigInt

Is it is similar situation to Data.Int64?

purescript-contrib/purescript-int64#8

@martyall
Copy link
Contributor

If i understand correctly, there is no hard max on the size of BigInt, it's limited only by memory of the machine. In which case I think it makes sense to just use the absolute value like you would for the integers. I have confirmed that these tests pass in this case.

@gbagan
Copy link
Contributor

gbagan commented Oct 9, 2023

Sorry to reopen a closed issue.

I think the fixed implement of degree is not correct even if the tests pass.

export const biDegree = (x) => {
  return x < 0n ? -x : x;
}

This is because the method degree must return an Int (not a BigInt) and the fixed implementation returns a JavascriptBigInt (with Purescript Type Int)
This can be a problem if, for example, we try to do something like degree (BigInt.fromInt 2) + 2 which will be well typed but will cause a runtime error (because Javascript does not allow to add a BigInt and an Int

Actually, I think there is no way to correctly implement degree (without changing the type of the method) but at least the previous implementation returns the good type.

@JordanMartinez
Copy link
Member

Given the above, I'm reopening this.

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