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

Create standardized, interpretable exceptions for error reporting #420

Closed
tdanford opened this Issue Oct 15, 2014 · 8 comments

Comments

Projects
5 participants
@tdanford
Contributor

tdanford commented Oct 15, 2014

I think that #297 is indicative of a slightly larger problem -- the way it manifests is that, when you run partitionAndJoin with some non-mappable records (i.e. records that can't be turned into a ReferenceRegion), you get a NullPointerException being thrown by an un-intelligible piece of code.

The challenge isn't just to catch this error condition earlier, it's to throw exceptions whose names/messages are interpretable to the end-user. So, for example, we could have an UnmappedException exception, which is thrown whenever "something that should be mappable, isn't."

Other ideas for exceptions --

  • invalid sample identifier
  • insufficient read coverage (or data coverage?)

thoughts?

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 15, 2014

Member

@tdanford this sounds like a heavyweight fix where a lightweight fix would do. If we throw an NPE when we create a reference mapping for a read that is unmapped, it would be sufficient check whether the read is mapped, and to throw an IllegalArgumentException if the read is unmapped which clearly states that the read is unmapped.

To clarify my opinions, I don't have any problems with custom exceptions; it's just that standardized, interpretable exceptions framework sounds like SkyNet for exceptions, and is probably out of scope for us to build.

Member

fnothaft commented Oct 15, 2014

@tdanford this sounds like a heavyweight fix where a lightweight fix would do. If we throw an NPE when we create a reference mapping for a read that is unmapped, it would be sufficient check whether the read is mapped, and to throw an IllegalArgumentException if the read is unmapped which clearly states that the read is unmapped.

To clarify my opinions, I don't have any problems with custom exceptions; it's just that standardized, interpretable exceptions framework sounds like SkyNet for exceptions, and is probably out of scope for us to build.

@tdanford

This comment has been minimized.

Show comment
Hide comment
@tdanford

tdanford Oct 15, 2014

Contributor

Yeah, I initially used an IAE for the other fix.

I don't think this needs to be "skynet," just some standardized bioinformatics-specific names for exceptions that we could throw. Otherwise, you're packing it all into the message of the IllegalArgumentException and (it seems to me) you'll end up regex'ing it out sometimes.

I dunno. Just a thought.

Contributor

tdanford commented Oct 15, 2014

Yeah, I initially used an IAE for the other fix.

I don't think this needs to be "skynet," just some standardized bioinformatics-specific names for exceptions that we could throw. Otherwise, you're packing it all into the message of the IllegalArgumentException and (it seems to me) you'll end up regex'ing it out sometimes.

I dunno. Just a thought.

@tdanford tdanford changed the title from Create standardized, interpretable exceptions framework for error reporting to Create standardized, interpretable exceptions for error reporting Oct 15, 2014

@tdanford

This comment has been minimized.

Show comment
Hide comment
@tdanford

tdanford Oct 15, 2014

Contributor

I took the word "framework" out of the title, does that sound a little less heavyweight now? :-D

It's basically about what you want to dispatch on. There are a lot of things that could throw an NPE inside some of this code.

Contributor

tdanford commented Oct 15, 2014

I took the word "framework" out of the title, does that sound a little less heavyweight now? :-D

It's basically about what you want to dispatch on. There are a lot of things that could throw an NPE inside some of this code.

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Oct 15, 2014

Member

+1; I think this:

I don't think this needs to be "skynet," just some standardized bioinformatics-specific names for exceptions that we could throw. Otherwise, you're packing it all into the message of the IllegalArgumentException and (it seems to me) you'll end up regex'ing it out sometimes.

Is a reasonable goal.

Member

fnothaft commented Oct 15, 2014

+1; I think this:

I don't think this needs to be "skynet," just some standardized bioinformatics-specific names for exceptions that we could throw. Otherwise, you're packing it all into the message of the IllegalArgumentException and (it seems to me) you'll end up regex'ing it out sometimes.

Is a reasonable goal.

@tdanford

This comment has been minimized.

Show comment
Hide comment
@tdanford

tdanford Oct 15, 2014

Contributor

This commit c8486eb has an example of what I'm intending.

Contributor

tdanford commented Oct 15, 2014

This commit c8486eb has an example of what I'm intending.

@calvertj

This comment has been minimized.

Show comment
Hide comment
@calvertj

calvertj Dec 24, 2014

Contributor

Any idea of what exceptions you would like? I think we could fold:
adam-core/src/main/scala/org/bdgenomics/adam/exceptions/UnmappedException.scala
into a
adam-core/src/main/scala/org/bdgenomics/adam/exceptions/Exceptions.scala file.
Or would you prefer a file per exception?

Contributor

calvertj commented Dec 24, 2014

Any idea of what exceptions you would like? I think we could fold:
adam-core/src/main/scala/org/bdgenomics/adam/exceptions/UnmappedException.scala
into a
adam-core/src/main/scala/org/bdgenomics/adam/exceptions/Exceptions.scala file.
Or would you prefer a file per exception?

@laserson

This comment has been minimized.

Show comment
Hide comment
@laserson

laserson Dec 26, 2014

Contributor

I like single file, as long as it doesn't get unwieldy

Contributor

laserson commented Dec 26, 2014

I like single file, as long as it doesn't get unwieldy

@fnothaft

This comment has been minimized.

Show comment
Hide comment
@fnothaft

fnothaft Mar 2, 2017

Member

Closing as won't fix.

Member

fnothaft commented Mar 2, 2017

Closing as won't fix.

@fnothaft fnothaft closed this Mar 2, 2017

@heuermh heuermh added this to Completed in Release 0.23.0 Mar 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment