Skip to content
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

feat(api, shared-data): add correctionByVolume to liquid class schema and definitions #16972

Merged
merged 13 commits into from
Dec 2, 2024

Conversation

jbleon95
Copy link
Contributor

@jbleon95 jbleon95 commented Nov 25, 2024

Overview

Closes AUTH-868.

This PR adds a correctionByVolume field to liquid class definitions in the schema, pydantic models and PAPI classes for liquid class properties. This value is similar to other byVolume properties in it's schema structure and usage for volume interpolation, but unlike the other properties allows negative values to be keyed to volume.

For the existing water definition, there is no correction so singular values of [0.0, 0.0] were put in so that all volumes produce this result.

Test Plan and Hands on Testing

Use existing schema validation and integration tests.

Changelog

  • added correctionByVolume property to schema, shared data models, and PAPI classes
  • added correctionByVolume values to water.json

Review requests

Would we want to represent water's default in another way? Any value could be used for a single interpolation point and 0.0 seems to be the most straightforward way of representing that, but I'd be open to other suggestions.

Risk assessment

Low.

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.00%. Comparing base (d5b7e61) to head (ee6ddc3).
Report is 59 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #16972      +/-   ##
==========================================
+ Coverage   73.90%   74.00%   +0.09%     
==========================================
  Files          43       43              
  Lines        3231     3250      +19     
==========================================
+ Hits         2388     2405      +17     
- Misses        843      845       +2     
Flag Coverage Δ
shared-data 74.00% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...red_data/liquid_classes/liquid_class_definition.py 95.52% <100.00%> (+0.17%) ⬆️

... and 4 files with indirect coverage changes

@jbleon95 jbleon95 marked this pull request as ready for review December 2, 2024 15:10
@jbleon95 jbleon95 requested review from a team as code owners December 2, 2024 15:10
@sfoster1
Copy link
Member

sfoster1 commented Dec 2, 2024

There's a lot of stuff in here talking about "this adds a volume correction" but it seems like none of it says what a volume correction is and what sort of numbers one might expect in here - might be worth having a sentence in a schema description somewhere.

@@ -31,6 +31,9 @@
LiquidHandlingPropertyByVolume = Sequence[Tuple[_NonNegativeNumber, _NonNegativeNumber]]
"""Settings for liquid class settings that are interpolated by volume."""

CorrectionByVolume = Sequence[Tuple[_NonNegativeNumber, _Number]]
"""Settings for correctionByVolume, which unlike other `byVolume` properties allows negative values with volume."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be helpful to explain what these new values mean -- e.g., is this correction ADDED or SUBTRACTED from the nominal value?

@jbleon95 jbleon95 merged commit 4c7a409 into edge Dec 2, 2024
66 checks passed
@jbleon95 jbleon95 deleted the correction_by_volume branch December 2, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants