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

Bound stream computed field with physical limits #82

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

t-sasatani
Copy link
Collaborator

@t-sasatani t-sasatani commented Nov 27, 2024

  • Sets limits on the computed ADC voltage value.
  • The computed voltage is set to zero -1 if it exceeds the limits.
  • This doesn't affect the raw metadata value.

📚 Documentation preview 📚: https://miniscope-io--82.org.readthedocs.build/en/82/

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 12058948228

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 77.286%

Totals Coverage Status
Change from base Build 11828727123: 0.07%
Covered Lines: 1065
Relevant Lines: 1378

💛 - Coveralls

@sneakers-the-rat
Copy link
Collaborator

seems fine to me, i guess my question is when does this occur? seems like if the device is giving us impossible values that's a problem with the device not the i/o software, and setting this to 0 could mask that problem. i trust ya but can i have it explained to me why it's preferable to have 0 in the recorded values rather than the value itself, which can be clamped in analysis?

battery_max_voltage: float = Field(
10.0,
description="Maximum voltage of the battery."
"Scaled battery voltage will be 0 if it is greater than this value",
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use -1 to make it unambiguous from the value actually being zero (which should never happen, but thinking about downstream data analysis)

@sneakers-the-rat
Copy link
Collaborator

sneakers-the-rat commented Dec 3, 2024

Also I think we are getting to the point where we want to have a separate ScaledValue model, having a bunch of fields named by convention isn't great and not very DRY.

like

T = TypeVar("T", bound = Union[int, float])

class ScaledValue(BaseModel, Generic[T]):
    factor: T = 1
    maximum: Optional[T] = None
    minimum: Optional[T] = None

    def scale(self, value: T) -> T:
        return np.clip(value * factor, self.minimum, self.maximum)

So then for example

class StreamConfig(BaseModel):
    battery_voltage: ScaledValue[float] = ScaledValue(factor = 10, maximum = 20)
    vin_voltage: ScaledValue[float] = ScaledValue(factor=1000, maximum = 20)

where idk we could compute that ScaledValue's factor param from other values like the bit depth if we want to or else make a subclass of ScaledValue.

battery_voltage:
  factor: 100
  maximum: 10
# ...
cfg = StreamConfig()
cfg.battery_voltage.scale(100)

@t-sasatani t-sasatani closed this Dec 13, 2024
@t-sasatani t-sasatani deleted the bound-computed-field branch December 13, 2024 21:08
@t-sasatani t-sasatani restored the bound-computed-field branch December 13, 2024 21:11
@t-sasatani t-sasatani reopened this Dec 13, 2024
@t-sasatani t-sasatani force-pushed the bound-computed-field branch from e6a24de to c713ac7 Compare February 3, 2025 01:31
@coveralls
Copy link
Collaborator

coveralls commented Feb 3, 2025

Coverage Status

coverage: 79.162% (+0.05%) from 79.112%
when pulling 42c564f on bound-computed-field
into 56397e9 on main.

@t-sasatani t-sasatani force-pushed the bound-computed-field branch from 32927fc to 42c564f Compare February 3, 2025 03:16
Comment on lines +135 to +159
_adc_scale: ADCScaling = PrivateAttr()
_battery_voltage_scaling: ScaledValue = PrivateAttr()
_input_voltage_scaling: ScaledValue = PrivateAttr()

def __init__(self, adc_scale: ADCScaling, **data: dict):
super().__init__(**data)
self._adc_scale = adc_scale
self._battery_voltage_scaling = ScaledValue(
scaling_factor=(
1
/ (2**self._adc_scale.bitdepth)
* self._adc_scale.ref_voltage
* self._adc_scale.battery_div_factor
),
maximum=self._adc_scale.battery_max_voltage,
)
self._input_voltage_scaling = ScaledValue(
scaling_factor=(
1
/ (2**self._adc_scale.bitdepth)
* self._adc_scale.ref_voltage
* self._adc_scale.vin_div_factor
),
maximum=self._adc_scale.vin_max_voltage,
)
Copy link
Collaborator Author

@t-sasatani t-sasatani Feb 3, 2025

Choose a reason for hiding this comment

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

[Notes for whenever I can come back] Not done yet. Doing this for every header input feels like a waste and also makes the header-filling process inconsistent. So, it is probably better to define a helper class that is set once with the device config and converts StreamBufferHeader into scaled values.

@t-sasatani
Copy link
Collaborator Author

i guess my question is when does this occur? seems like if the device is giving us impossible values that's a problem with the device not the i/o software, and setting this to 0 could mask that problem.

This mostly happens when the channel or DAQ hardware introduces some bit errors. We're using uint32 to send 8-bit ADC values to simplify the buffer structure, so any flip in the higher bits messes up the metadata plot. I'll change the mask value to -1.

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.

3 participants