-
Notifications
You must be signed in to change notification settings - Fork 12
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
Filter prefixes #293
Filter prefixes #293
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve this, but if you really want to make me happy you can add a CLI method
remove(msdf1, msdf2)
msdf1.remove_mappings(msdf2)
write(msdf1)
Your wish is my command!
should work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, left a few comments but nothing that blocks merging. THANKS
@main.command() | ||
@input_argument | ||
@click.option( | ||
"--remove-map", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, we can later consider making this multivalued (not now). Thank you!
@@ -360,6 +360,8 @@ def filter_file(input: str, output: TextIO, **kwargs) -> MappingSetDataFrame: | |||
for exp in v[1:]: | |||
query += " OR " | |||
query += k + " LIKE '" + exp + "') " | |||
else: | |||
query += ") " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is new, I assume there was no test for the else
case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was missing and became apparent when only 1 value to a param was given for e.g. --subject_id ABCD:1234
instead of multiple --subject_id ABCD:1234 --subject_id DEFG:4567
. The missing )
that's added in the code above threw an error. There was no test to check this.
filter_prefixes
: This filters themsdf.df
to contain only those mappings with prefixes that are present in a list provided by the user.remove_mappings()
to MSDF class. Addresses one checkbox in Lexical mapping analysis and feedback/review monarch-initiative/mondo-ingest#49