ENA-6078: Fixing partiality #159
Conversation
raskoleinonen
left a comment
raskoleinonen
left a comment
There was a problem hiding this comment.
First relatively fast review.
|
|
||
| private boolean leftPartial; | ||
| private boolean rightPartial; | ||
| private boolean fivePrime; |
There was a problem hiding this comment.
Change to:
- fivePrimePartial
- threePrimePartial
| private Long beginPosition; | ||
| private Long endPosition; | ||
|
|
||
| private boolean threePrime; |
There was a problem hiding this comment.
We should have the partiality in one place only or they may have conflicting values. If we have getters and setters in both CompoundLocation and Location then CompoundLocation should call the Location getters and setters and not have partiality fields itself. In this case CompoundLocation partiality setting should fail if there are no Locations yet.
| if (translationResult.isFixedLeftPartial()) { | ||
| if (!cds.getLocations().isComplement()) { | ||
| cds.getLocations().setLeftPartial(translator.isLeftPartial()); | ||
| cds.getLocations().setFivePrime(translator.isLeftPartial()); |
There was a problem hiding this comment.
Potentially unsafe. Best would be to review change Translation left and right partiality as well.
| cds.getLocations().setFivePrime(translator.isLeftPartial()); | ||
| } else { | ||
| cds.getLocations().setRightPartial(translator.isLeftPartial()); | ||
| cds.getLocations().setThreePrime(translator.isLeftPartial()); |
There was a problem hiding this comment.
Potentially unsafe. Best would be to review change Translation left and right partiality as well.
| if (translationResult.isFixedRightPartial()) { | ||
| if (!cds.getLocations().isComplement()) { | ||
| cds.getLocations().setRightPartial(translator.isRightPartial()); | ||
| cds.getLocations().setThreePrime(translator.isRightPartial()); |
There was a problem hiding this comment.
Potentially unsafe. Best would be to review change Translation left and right partiality as well.
| firstContig = false; | ||
| } | ||
| FeatureLocationWriter.renderLocation(block, contig, false, false); | ||
| FeatureLocationWriter.renderLocation(block, contig); |
There was a problem hiding this comment.
We never want to write partiality for CO lines. The old code prevented this completely. Now we will write partiality if the location has it.
| firstContig = false; | ||
| } | ||
| FeatureLocationWriter.renderLocation(block, contig, false, false); | ||
| FeatureLocationWriter.renderLocation(block, contig); |
There was a problem hiding this comment.
We never want to write partiality for Contig lines. The old code prevented this completely. Now we will write partiality if the location has it.
| assertEquals("complement(467..>468)", test("complement(467..>468)")); | ||
| assertEquals("complement(467^>468)", test("complement(467^>468)")); | ||
|
|
||
| // Note that 3' prime end (right) partiality is lost. |
| assertEquals("complement(<467)", test("complement(<>467)")); | ||
| assertEquals("complement(<467)", test("complement(<467)")); | ||
| // Note that 3' prime end (right) partiality is lost. | ||
| assertEquals("complement(<467)", test("complement(<467..>467)")); |
There was a problem hiding this comment.
I wonder where we lost one of the partialities.
|
|
||
| location = "complement(join(2182966..2183014,2183124..>2183128))"; | ||
| assertEquals(location, test(location)); | ||
| assertTrue(leftPartial(location)); |
There was a problem hiding this comment.
We should not be using left and right partial terms anymore anywhere.
Created FeatureLocationParser for parsing the CompoundLocation
Added 2 fields in Location object, this is used to write the locations
Updated FeatureLocationWriter to write location partiality using Location object
Failing tests are not committed in this branch to keep the PR simple
We can test using FeatureLocationsMatcherTest
Need to update LocationToStringCoverter (Note to self)