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

[query] ErrorIDs #10655

Merged
merged 14 commits into from Jul 14, 2021
Merged

[query] ErrorIDs #10655

merged 14 commits into from Jul 14, 2021

Conversation

ammekk
Copy link
Contributor

@ammekk ammekk commented Jul 12, 2021

Providing users with more helpful errors

@johnc1231 johnc1231 changed the title ErrorIDs [query] ErrorIDs Jul 13, 2021
Comment on lines 326 to 334
// def get(cb: EmitCodeBuilder, errorMsg: String = s"expected non-missing"): A = {
// get(cb, errorMsg, Code.intValue(Code.boxInt(ErrorIDs.NO_ERROR)))
// }
//
// def get(cb: EmitCodeBuilder, errorMsg: String = s"expected non-missing", errorID: Code[Int] = const(ErrorIDs.NO_ERROR)): A =
// handle(cb, cb._fatalWithError(errorID, errorMsg))
//
// def get(cb: EmitCodeBuilder, errorMsg: Code[String]): A =
// get(cb, errorMsg, Code.intValue(Code.boxInt(ErrorIDs.NO_ERROR)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete all of these.

Comment on lines 77 to 83
registerIR2("isSubset", TSet(tv("T")), TSet(tv("T")), TBoolean) { (_, s, w, _) =>
val t = s.typ.asInstanceOf[TSet].elementType
val a = genUID()
val x = genUID()
StreamFold(ToStream(s), True(), a, x,
// FIXME short circuit
ApplySpecial("land", FastSeq(), FastSeq(Ref(a, TBoolean), contains(w, Ref(x, t))), TBoolean))
ApplySpecial("land", FastSeq(), FastSeq(Ref(a, TBoolean), contains(w, Ref(x, t))), TBoolean, ErrorIDs.NO_ERROR))
Copy link
Contributor

Choose a reason for hiding this comment

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

You have an error id available here. Try adding a test for this error to make sure it's a HailUserError

Comment on lines +63 to +64
val r = isValid(tl, Seq(contig, pos), ErrorIDs.NO_ERROR)
val p = getRef(tl, Seq(contig, pos, before, after), ErrorIDs.NO_ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

You have an errorId available here

Comment on lines 48 to 52
cb.ifx(n0 cne n1, cb._fatalWithError(ErrorIDs.NO_ERROR, "hail.nd.solve: matrix a must be square."))

val IndexedSeq(n, nrhs) = bColMajor.shapes(cb)

cb.ifx(n0 cne n, cb._fatal("hail.nd.solve: Solve dimensions incompatible"))
cb.ifx(n0 cne n, cb._fatalWithError(ErrorIDs.NO_ERROR, "hail.nd.solve: Solve dimensions incompatible"))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can know the error here. Pass the error id into linear_solve

Comment on lines 177 to 183
cb += Code._fatalWithID[Unit](
const("locus_windows: missing value for 'coord_expr' at row ")
.concat((offset + i).toS)),
.concat((offset + i).toS), errorID),
{ sc =>
val currentCoord = cb.newLocal[Double]("locuswindows_coord_i", sc.asDouble.doubleCode(cb))
cb.ifx(lastCoord > currentCoord,
cb += Code._fatal[Unit]("locus_windows: 'coord_expr' must be in ascending order within each contig."),
cb += Code._fatalWithID[Unit]("locus_windows: 'coord_expr' must be in ascending order within each contig.", errorID),
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace these with cb._fatalWithIDs instead of doing cb += Code....

@@ -2515,7 +2506,7 @@ class Emit[C](

case x =>
if (fallingBackFromEmitI) {
fatal(s"ir is not defined in emit or emitI $x")
fatal( s"ir is not defined in emit or emitI $x")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't add extra space here.

@@ -779,4 +782,6 @@ final case class ShuffleRead(

object ErrorIDs {
val NO_ERROR = -1

private object NO_Error
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

val behavior = identifier(it) match {
case "AssertSameLength" => ArrayZipBehavior.AssertSameLength
case "TakeMinLength" => ArrayZipBehavior.TakeMinLength
case "TakeMinLength" => ArrayZipBehavior.TakeMinLength
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete duplication.

Copy link
Contributor

@johnc1231 johnc1231 left a comment

Choose a reason for hiding this comment

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

Really great work!

@danking danking merged commit 9eae0da into hail-is:main Jul 14, 2021
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

3 participants