[BI-2065] - Experiment overwrite missing phenotype validations#340
[BI-2065] - Experiment overwrite missing phenotype validations#340
Conversation
nickpalladino
left a comment
There was a problem hiding this comment.
I looked a little more and saw that NAs are being skipped in validateObservationValue so the point I brought up shouldn't be an issue. I'll re-review after the changes you're working on are done.
mlm483
left a comment
There was a problem hiding this comment.
I found two overwrite scenarios which I think need to be handled differently, although that could happen in a different story.
Empty timestamps cause unhandled exception when overwriting
To reproduce:
- Upload germplasm.xls, ontology.xls and minimal_experiment.xlsx. This experiment file has a TS column with some empty values.
- After the upload succeeds, download the experiment file with timestamps for the "Some Phenotypes with TS" experiment you just created.
- Reupload this file (you can change some of the pheno values if you want).
- The empty timestamps, which were successfully uploaded initially, result in an unhandled
DateTimeParseException.
Desired Behavior:
- If empty timestamps are allowed in the initial upload, they should be allowed in the overwrite.
"NA" values are treated as "no change" when overwriting
To reproduce:
- Upload germplasm.xls, ontology.xls and experiment.xlsx.
- Download the resulting experiment.
- Change some pheno values to "NA" and reupload to overwrite.
- Upload succeeds, but pheno values changed to "NA" are treated as if they are unchanged.
Desired behavior:
- Per Shawn's comment, "NA" should be saved to the database when overwriting.
| } else if (existingObsByObsHash.containsKey(importHash) && !isObservationMatched(importHash, importObsValue, phenoCol, rowNum)) { | ||
|
|
||
| // different data means validations still need to happen | ||
| // TODO consider moving these two calls into a separate method since called twice together |
There was a problem hiding this comment.
Is this referring to validateObservationValue and validateTimeStampValue?
There was a problem hiding this comment.
yes, since we have validation in two places in the method (one for the overwrite case and one for the non-overwrite case, it feels like it should be cleaned up in some manner)
I can look into seeing how extensive those changes might be, since it would be neater to have it all handled here if it doesn't increase scope too much rather than having to list still failing overwrite conditions. |
Handled case where present observation but blank associated timestamp led to an unhandled attempt to parse said blank timestamp
|
@HMS17 The I'm seeing odd behavior when overwriting observation values with the string "NA", it isn't shown in the preview and seems to be converted to |
mlm483
left a comment
There was a problem hiding this comment.
After getting bi-api to build with the updated tablesaw dependency, everything is working as expected.
Description
Story: BI-2065 - Experiment overwrite does not do all data validations on phenotype data
Updated logic in ExperimentProcessor.java:validateObservations to run validations in the overwrite case
Dependencies
n/a
Testing
Checklist: