ACTIN-1057: Filter NaN likelihoods from CUPPA when converting in ORANGE #569
Conversation
| @@ -83,9 +79,13 @@ private static List<CuppaPrediction> extractSortedProbabilities(@NotNull CuppaPr | |||
| .altSjCohortClassifier(probabilitiesByClassifier.get(ClassifierName.ALT_SJ)) | |||
| .build(); | |||
|
|
|||
| cuppaPredictionsOrangeFormat.add(prediction); | |||
| // If a classifier has no data for a specific cancer type we should remove it completely, see ACTIN-1057 | |||
There was a problem hiding this comment.
Feels a bit unusual to refer to a ticket in code, unless its an open TODO that needs followup.
There was a problem hiding this comment.
indeed I'd normally just include this sort of thing in the commit message. Future devs can annotate the code to see it.
There was a problem hiding this comment.
Do note that I added a comment because the implementation choice may not be optimal. We could for instance also expose a wider CUPPA datamodel in ORANGE and leave to downstream how to handle NaN likelihoods, but considered that unnecessary at this point
| expectedPredictionsByCancerType.put(expectedPredictionMelanoma.cancerType(), expectedPredictionMelanoma); | ||
| expectedPredictionsByCancerType.put(expectedPredictionSkinOther.cancerType(), expectedPredictionSkinOther); | ||
| expectedPredictionsByCancerType.put(expectedPredictionProstate.cancerType(), expectedPredictionProstate); | ||
| expectedPredictionsByCancerType.put("Bone/Soft tissue: Cartilaginous neoplasm", null); |
There was a problem hiding this comment.
Where is the null setup in the test data? ... oh ok, I see that it was always there but its actually missing values in the input file from CUPPA and not explicit NaN's.
There was a problem hiding this comment.
Correct, that's what tricked us in the first place :)
Have tested on data-vm and confirmed this ORANGE version creates a JSON file which can be ingested into ACTIN (no NaN exception anymore).