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

Improve error message when we can't find a ReferenceRegion for a contig #582

Closed
fnothaft opened this Issue Feb 18, 2015 · 3 comments

Comments

Projects
None yet
4 participants
@fnothaft
Member

fnothaft commented Feb 18, 2015

Seen on the mailing list. If calling ReferenceRegion(contig: NucleotideContigFragment) returns None, we throw a nondescript error.

@fnothaft fnothaft added the bug label Feb 18, 2015

@fnothaft fnothaft self-assigned this Feb 18, 2015

@InvisibleTech

This comment has been minimized.

Show comment
Hide comment
@InvisibleTech

InvisibleTech May 21, 2015

Contributor

Is this issue still open? I was looking for a starting point to help out and noticed this issue has an assignee but was also labeled "pick me up".

Contributor

InvisibleTech commented May 21, 2015

Is this issue still open? I was looking for a starting point to help out and noticed this issue has an assignee but was also labeled "pick me up".

@heuermh

This comment has been minimized.

Show comment
Hide comment
@heuermh

heuermh May 13, 2016

Member

@InvisibleTech go for it! We're not too strict about the Assignee label.

Member

heuermh commented May 13, 2016

@InvisibleTech go for it! We're not too strict about the Assignee label.

@InvisibleTech

This comment has been minimized.

Show comment
Hide comment
@InvisibleTech

InvisibleTech May 29, 2016

Contributor

@fnothaft @heuermh

Is there something that explains what defines a valid fragment? Also do you have perferences regarding how to fail at this point? Default quietly to an empty value? Raise a RuntimeException? Meaning what is the style of error processing you follow here in this project?

Requirements question. I am looking at fixing this issue with some defensive coding in:

org.bdgenomics.adam.converters.FragmentCollector#apply
Normally in a Java world I would do something like throw a RuntimeException like IllegalArgumentException but I am not sure in FP Scala land if this is legit.

Looking at the code in:

org.bdgenomics.adam.models.ReferenceRegion#apply

I see that if any of the following referenced fields aren't valued we would get None because of the generators used in the for-comprehension:

  contig <- Option(fragment.getContig)
  contigName <- Option(contig.getContigName)
  startPosition <- Option(fragment.getFragmentStartPosition)

So which of these fields is required to properly create a ReferenceRegion? Is this an error or is it more legit for me to silently create some empty Seq[(ReferenceRegion, String)] if ReferenceRegion(fragment) produces a None?

Contributor

InvisibleTech commented May 29, 2016

@fnothaft @heuermh

Is there something that explains what defines a valid fragment? Also do you have perferences regarding how to fail at this point? Default quietly to an empty value? Raise a RuntimeException? Meaning what is the style of error processing you follow here in this project?

Requirements question. I am looking at fixing this issue with some defensive coding in:

org.bdgenomics.adam.converters.FragmentCollector#apply
Normally in a Java world I would do something like throw a RuntimeException like IllegalArgumentException but I am not sure in FP Scala land if this is legit.

Looking at the code in:

org.bdgenomics.adam.models.ReferenceRegion#apply

I see that if any of the following referenced fields aren't valued we would get None because of the generators used in the for-comprehension:

  contig <- Option(fragment.getContig)
  contigName <- Option(contig.getContigName)
  startPosition <- Option(fragment.getFragmentStartPosition)

So which of these fields is required to properly create a ReferenceRegion? Is this an error or is it more legit for me to silently create some empty Seq[(ReferenceRegion, String)] if ReferenceRegion(fragment) produces a None?

fnothaft added a commit to fnothaft/adam that referenced this issue Jul 20, 2016

[ADAM-582] Eliminate .get on option in FragmentCoverter.
Resolves #582. In the FragmentConverter, we had code that called .get on an
optional ReferenceRegion. If a NucleotideContigFragment didn't have a contig
associated with it, then we would call .get on the empty option, which would
throw an error. This commit changes that code to call map on the
Option[ReferenceRegion], and then changes the usage of that method so that it
is called in a flatMap. Additionally, I've added a test to cover this case.

@heuermh heuermh closed this in 7c2df60 Jul 20, 2016

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