-
Notifications
You must be signed in to change notification settings - Fork 6
Support additional unit types #7243
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
base: develop
Are you sure you want to change the base?
Conversation
…ty initial amount
| "count", "count"), | ||
| pcs(KindOfQuantity.Count, unit, 1.0, 2, "pcs", | ||
| Quantity.class, | ||
| "pcs", "pcs"), |
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 think singular here is "piece", but might want to confirm that with @labkey-hannah.
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.
Agreed, we should switch this to "piece"/"pieces"
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.
updated
| assertTrue(Unit.unit.isCompatible(Unit.bottle)); | ||
| assertTrue(Unit.unit.isCompatible(Unit.blocks)); | ||
| assertTrue(Unit.unit.isCompatible(Unit.box)); | ||
| assertTrue(Unit.unit.isCompatible(Unit.slides)); |
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.
There are some missing here. Not that I think there's risk or regression here, just for completeness.
| {name: 'S-kg', amount: 4.56, units: 'kg'}, | ||
| ], 'samples', sampleTypeMass, topFolderOptions, editorUserOptions); | ||
|
|
||
| // check for storedamount in g |
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.
check for raw amount in g and display amount in ug
| { | ||
| var ret = wrapColumn(alias, _rootTable.getColumn(AliquotVolume.name())); | ||
| ret.setLabel(ALIQUOT_VOLUME_LABEL); | ||
| ret.setLabel("Raw " + ALIQUOT_VOLUME_LABEL); |
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 good to use the label specified from the ExpMaterialTable.Column enum here an for the other new columns so the labels get properly reserved.
| { | ||
| SampleTypeAmountDisplayColumn columnInfo = new SampleTypeAmountDisplayColumn(this, Column.AvailableAliquotVolume.name(), Column.AliquotUnit.name(), AVAILABLE_ALIQUOT_VOLUME_LABEL, Collections.emptySet(), typeUnit); | ||
| columnInfo.setDisplayColumnFactory(colInfo -> new SampleTypeAmountPrecisionDisplayColumn(colInfo, typeUnit)); | ||
| columnInfo.setDescription("The total amount of this sample's aliquots that's available, in the display unit for the sample type, currently on hand."); |
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 doesn't parse very well. Perhaps "The total amount of this sample's available aliquots currently on hand, in the display unit for the sample type."
| public void addQueryFieldKeys(Set<FieldKey> keys) | ||
| { | ||
| super.addQueryFieldKeys(keys); | ||
| keys.add(FieldKey.fromParts(AvailableAliquotVolume)); |
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.
Same here AvailableAliquotVolume.fieldKey()
| QueryableInputs, | ||
| RawAliquotUnit, | ||
| RawAliquotVolume, | ||
| RawAvailableAliquotVolume, |
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.
Should set the labels for these fields here.
| TableInfo tableInfo = ExperimentService.get().getTinfoMaterial(); | ||
|
|
||
| ListUtils.partition(sampleAliquotAmountsList, 1000).forEach(sublist -> | ||
| ListUtils.partition(new ArrayList<>(withAmountsParents), 1000).forEach(sublist -> |
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.
Do you need a new array here? Can you not partition withAmountsParents?
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.
A list is needed, but withAmountsParents is a Collection.
| String totalUnitsStr; | ||
| if (!StringUtils.isEmpty(sampleTypeUnitsStr)) | ||
| totalUnitsStr = sampleTypeUnitsStr; | ||
| else if (hasSameAliquotUnit && !StringUtils.isEmpty(volumeUnits.get(0).unit)) // if all aliquots have the same unit, prefer it over parent's unit |
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.
Doesn't this lead to divergence from base units?
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.
Not really. In the case of an absent display unit, we don't know if aliquot unit or parent unit would be more likely to be in base unit. We don't do conversion when there is no sampleTypeUnitsStr.
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.
Oh right. Missed that this is the sampletypeUnitStr is empty case.
| } | ||
|
|
||
| private Map<Long, Pair<Integer, String>> getSampleAvailableAliquotCounts(Collection<Long> sampleIds, Collection<Long> availableSampleStates, boolean useRootMaterialLSID) throws SQLException | ||
| private Map<Long, Pair<Integer, String>> getSampleAvailableAliquotCounts(Collection<Long> sampleIds, Collection<Long> availableSampleStates) throws SQLException |
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.
Nice to get rid of the useRootMaterialLSID complexity.
Rationale
This pull request adds support for additional unit types in the sample type system, particularly extending the Count kind of quantity with new unit types (pcs, pack, blocks, slides, cells, box, kit, tests, bottle) and expanding mass units to include smaller units (ug, ng).
Related Pull Requests
Changes