feat: Molecular Devices SoftMax Pro - Enable Reduced-only file parsing#1016
Conversation
|
blocking/suggestion Note to self: There is probably some documentation that needs to be updated along with this change. |
janedance
changed the title
1c541b0 to
fbabb6d
Compare
There was a problem hiding this comment.
Hi Jane! Thank you so much for contributing!
Looking at this change, I think what we'll want to do for capturing reduced data only is a little different. It looks like this change is causing the reduced data to be captured as the raw data.
This causes us to report the reduced data as the raw data (you can see the reduced values are being reported as the "absorbance" values in the measurement document, whereas we want to report the reduced values as calculated data, since it's not really raw data!)
I think to do this, what we actually want to do is check the read type in TimeData and only read the raw data if they read type is "RAW" or "BOTH", and only read the reduced data if the type is "REDUCED" or "BOTH".
There is one more wrinkle specific to the ASM format, which is that the measurement document currently requires an "absorbance" value, and we don't have that value for the reduced only format.
I think the way to address this will be to, for the reduced only case, generate aTimeRawData with TimeMeasurementData with NEGATIVE_ZERO for the value and an ErrorDocument saying "Not reported"
Let me know if you have any questions!
…reduced data when appropriate
|
Thanks @nathan-stender for the detailed feedback, I appreciate it! I made the requested change e86c186 and found that it had a wider impact than I anticipated. I believe this is because reduced data was previously being read for "RAW" type. So, the test output changed for several cases. I want to be cautious here, since I didn't set out to change existing functionality. I would like to confirm with you:
If it does have the possibility of breaking existing usage, then I can change it back to continuing reading reduced data for "RAW" type. Let me know what you think! |
There was a problem hiding this comment.
You're right! I took a look at this, and it appears that the problem is that the RAW format can just... report reduced data as well 🤦. Confirmed this with our scientific consultant reviewing the existing files.
Given this, I think that we should keep the first part of what you added (reduced only creating empty data) but go back to always trying to read reduced data.
I tried that out (changing the conditional for reduced data to if reader.current_line_exists()) and fixed the changes in existing files while supporting the new case!
Other than that, a couple minor request and I think we'll be all set!
…rrent_line_exists()
…ersion (don't touch what passes tests)
|
Thanks @nathan-stender once more for the detailed feedback! 🙏 I've made the requested changes - let me know if you have any other suggestions |
|
@janedance this looks perfect! Thank you so much for the contribution! One last thing - feel free to add yourself to the |
You're very welcome! And thank you for the code reviews! I've added myself to the authors list :) |
### Added - Bio-Rad Bio-Plex Manager - Add support for statistics dimensions (#1029) - Agilent Gen5 - Add Conc/Dil to Sample document custom fields (#1030) - Roche Cedex BioHT - Added support for mg/L (#1028) - Thermo Fisher Scientific SkanIt - Support multiplates (#1025) - Beckman Vi-Cell Blu - Added get_unread data to parser (#1023) - Molecular Devices SoftMax Pro - Enable Reduced-only file parsing (#1016)
#1016) Updating the Softmax parser to pull in the following data: Group: Control Sample Wells Sample# Values Results MeanResults CVs CVresults Group: Matrix_Spike_1 Sample Wells Value R Result MeanResult CVs CvResult %SpikeRecovery Group: Matrix_Spike_2 Sample Well Value R Result MeanResult CVs CVResult %SpikeRecovery Group: Standards Sample Concentration ng/mL Wells Value MeanValue SD CVs CV_Result Group: Samples Sample Wells Value R Result MeanResult SD CVs CVresult I believe this requires adding support for `REDUCED` files, so that's what this PR is for. I think we also need the "Acceptance Criteria" data from the `Note' section, in which case I'll be following up with a larger PR to add some functionality there. --------- Co-authored-by: Nathan Stender <nathan.stender@benchling.com>
### Added - Bio-Rad Bio-Plex Manager - Add support for statistics dimensions (#1029) - Agilent Gen5 - Add Conc/Dil to Sample document custom fields (#1030) - Roche Cedex BioHT - Added support for mg/L (#1028) - Thermo Fisher Scientific SkanIt - Support multiplates (#1025) - Beckman Vi-Cell Blu - Added get_unread data to parser (#1023) - Molecular Devices SoftMax Pro - Enable Reduced-only file parsing (#1016)
2 participants
Updating the Softmax parser to pull in the following data:
I believe this requires adding support for
REDUCEDfiles, so that's what this PR is for.I think we also need the "Acceptance Criteria" data from the `Note' section, in which case I'll be following up with a larger PR to add some functionality there.