add predicate to GFF3Codec to give a chance to filter out some unused attributes #1575
+49
−9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lbergelson
requested changes
Nov 1, 2021
There was a problem hiding this comment.
@lindenb Thank you, this seems like a good idea. I have a few nitpicky comments, but look good overall.
| */ | ||
| public Gff3Codec setFilterOutAttribute(final Predicate<String> filterOutAttribute) { | ||
| /* check required keys are always kept */ | ||
| for(final String key : new String[] {Gff3Constants.PARENT_ATTRIBUTE_KEY,Gff3BaseData.ID_ATTRIBUTE_KEY,Gff3BaseData.NAME_ATTRIBUTE_KEY}) { |
There was a problem hiding this comment.
Nitpicky, but can you add spaces between these?
| @@ -200,7 +220,7 @@ private Gff3Feature decode(final LineIterator lineIterator, final DecodeDepth de | |||
| return attributes; | |||
| } | |||
|
|
|||
| static private Gff3BaseData parseLine(final String line, final int currentLine) { | |||
| private Gff3BaseData parseLine(final String line, final int currentLine) { | |||
There was a problem hiding this comment.
I'd probably keep this static and explicitly pass in the predicate
| @@ -217,6 +237,8 @@ static private Gff3BaseData parseLine(final String line, final int currentLine) | |||
| final int phase = splitLine.get(GENOMIC_PHASE_INDEX).equals(Gff3Constants.UNDEFINED_FIELD_VALUE) ? -1 : Integer.parseInt(splitLine.get(GENOMIC_PHASE_INDEX)); | |||
| final Strand strand = Strand.decode(splitLine.get(GENOMIC_STRAND_INDEX)); | |||
| final Map<String, List<String>> attributes = parseAttributes(splitLine.get(EXTRA_FIELDS_INDEX)); | |||
| /* remove attibutes if they fail 'acceptExtraFieldKey' */ | |||
There was a problem hiding this comment.
This comment no longer matches the variable name.
| * @param filterOutAttribute the predicate | ||
| * @return this codec | ||
| */ | ||
| public Gff3Codec setFilterOutAttribute(final Predicate<String> filterOutAttribute) { |
There was a problem hiding this comment.
I'm a bit torn if I like this as a fluent setter or instead if we should add this as an extra constructor parameter. That way we could make the predicate field final and reduce possible strange state changes, but it's not really that important either way.
| @@ -217,6 +237,8 @@ static private Gff3BaseData parseLine(final String line, final int currentLine) | |||
| final int phase = splitLine.get(GENOMIC_PHASE_INDEX).equals(Gff3Constants.UNDEFINED_FIELD_VALUE) ? -1 : Integer.parseInt(splitLine.get(GENOMIC_PHASE_INDEX)); | |||
| final Strand strand = Strand.decode(splitLine.get(GENOMIC_STRAND_INDEX)); | |||
| final Map<String, List<String>> attributes = parseAttributes(splitLine.get(EXTRA_FIELDS_INDEX)); | |||
| /* remove attibutes if they fail 'acceptExtraFieldKey' */ | |||
| attributes.keySet().removeIf(KEY->this.filterOutAttribute.test(KEY)); | |||
There was a problem hiding this comment.
I think this can be the less verbose:
attributes.keySet().removeIf(this.filterOutAttribute);
| @@ -9,8 +9,8 @@ | |||
| import java.util.Map; | |||
|
|
|||
| public class Gff3BaseData { | |||
| private static final String ID_ATTRIBUTE_KEY = "ID"; | |||
| private static final String NAME_ATTRIBUTE_KEY = "Name"; | |||
| static final String ID_ATTRIBUTE_KEY = "ID"; | |||
There was a problem hiding this comment.
Should these move to the constants class?
Co-authored-by: Louis Bergelson <louisb@broadinstitute.org>
Codecov Report
@@ Coverage Diff @@
## master #1575 +/- ##
===============================================
- Coverage 69.841% 69.804% -0.037%
- Complexity 9633 9639 +6
===============================================
Files 702 702
Lines 37611 37618 +7
Branches 6108 6088 -20
===============================================
- Hits 26268 26259 -9
- Misses 8897 8907 +10
- Partials 2446 2452 +6
|
|
@lbergelson thank you for your review. I moved the |
lbergelson
approved these changes
Dec 8, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When using a GFF3Reader with DecodeDepth==DEEP, it may use a large amount of memory with attributes that will never be used ("version" ,"tag", etc...). This PR gives the GFF3Codec a chance to set a
Predicate<String>to only keep a defined set of attributes.the private attribute of ID_ATTRIBUTE_KEY and NAME_ATTRIBUTE_KEY Gff3BaseData was removed to check if the predicate does not remove them.
a new method
setFilterOutAttributewas added to GFF3Codecthe
staticattribute of GFF3Codec.parseLine was removedI added a test codecFilterOutFieldsTest
Things to think about before submitting: