-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fix ASCN namespace import #9156
Conversation
Kudos, SonarCloud Quality Gate passed! |
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.
Would be nice to have some tests to prevent these types of regression
@onursumer I agree. But I think this is out of scope for this PR. I was not involved in implementation of the ASCN feature. Perhaps we can make an issue? However, it is good to merge this PR asap since it is resulting in errors in frontend e2e-localdb tests. |
@@ -73,7 +73,7 @@ | |||
private Set<String> filteredMutations = new HashSet<String>(); | |||
private Set<String> namespaces = new HashSet<String>(); | |||
private Pattern SEQUENCE_SAMPLES_REGEX = Pattern.compile("^.*sequenced_samples:(.*)$"); | |||
private final String ASCN_NAMESPACE = "ascn"; | |||
private final String ASCN_NAMESPACE = "ASCN"; |
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.
@averyniceday can see any problem with this viz-a-viz legacy data? i.e. it was working before with lowercasing, now it won't?
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.
No, not really. The manual describes that ASCN should be represented with capitals in the meta_mutations_extended.txt file. The lowercase conversion happens in the backend and ASCN_NAMESPACE matched this converted value. In a previous PR I have removed this conversion to lowercase. This fix will correct the match to the original case present in the meta_mutations_extended.txt file.
TLDR Legacy data is only affected when using lowecase ascn in the metafile. This is not as described in the documentation and I consider this as making use a unsupported feature.
Problem
Namespace import of ASCN columns from MAF file was broken in previous commit because the namespace match is now case sensitive.
Solution
Correct case for namespace name used to decide whether to import ASCN columns.