147 call transformers for temp #157

jamienoss · 2024-08-29T18:35:56Z

Resolves #147

jamienoss · 2024-08-29T18:46:09Z

evolver/calibration/standard/tests/test_fitting.py

+@pytest.fixture()
+def tmp_calibration_dir(monkeypatch, tmp_path):
+    monkeypatch.setattr(evolver.settings.settings, "ROOT_CALIBRATOR_FILE_STORAGE_PATH", tmp_path)


Semi-unrelated, but I noticed that the tests were creating persisting calibration files.

jamienoss · 2024-08-29T18:47:37Z

evolver/hardware/standard/temperature.py


    class Output(SerialDeviceOutputBase, SensorDriver.Output):
-        temperature: float = Field(None, description="Sensor temperature in degrees celcius")
+        temperature: float = Field(None, description="Sensor temperature in degrees Celsius")


jamienoss · 2024-08-29T18:58:42Z

@amitschang Hey, do you mind taking a preliminary look at this, please?

Main design aspect is that the vial transformers are {vial: Transformer()} instead of the vial list/dict being a attr of a specific "multi-vial" transformer.

jamienoss · 2024-08-29T19:00:02Z

evolver/calibration/standard/polyfit.py

+        input_transformer: dict[int, ConfigDescriptor | LinearTransformer | None] | None = None
+        output_transformer: dict[int, ConfigDescriptor | LinearTransformer | None] | None = None
+
+    def run_calibration_procedure(self, data: dict):


FYI this is just a place-holder.

jamienoss · 2024-08-29T19:00:50Z

evolver/calibration/standard/polyfit.py

+        input_transformer: dict[int, ConfigDescriptor | LinearTransformer | None] | None = None
+        output_transformer: dict[int, ConfigDescriptor | LinearTransformer | None] | None = None


Main feature.

jamienoss · 2024-08-29T19:04:04Z

evolver/calibration/standard/polyfit.py

+            if save:
+                calibration_data[transformer].save()


codecov · 2024-08-29T19:06:17Z

Codecov Report

Attention: Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
evolver/calibration/standard/polyfit.py	64.28%	5 Missing ⚠️
evolver/hardware/interface.py	75.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

jamienoss · 2024-08-29T19:37:44Z

scripts/gen_default_cali_files.py

+    # coefficients = np.loadtxt("/Users/jamienoss/Downloads/TEMP_20190201.txt", delimiter=',')
+
+    # https://github.com/FYNCH-BIO/evolver/blob/master/evolver/calibrations.json
+    with open("/Users/jamienoss/Downloads/calibrations.json") as fp:


Needs to go, use requests instead.

amitschang · 2024-09-03T19:04:04Z

Seems good, Just a couple impressions:

I was imagining that the calibration procedure would be most flexible if it stored (or additionally had the option to store) the points of raw mapping to real, since then one could swap out fit functions, but here the coefficients from the fit look to be what is in the config file, if I'm reading it right. Any comment?
In the temperature driver the code allows both global and vial, but I'm not sure there is a good scope for global coefficients so might be simpler to just require per-vial there.

jamienoss · 2024-09-03T19:21:31Z

@amitschang thanks for the prelim review.

I was imagining that the calibration procedure would be most flexible if it stored (or additionally had the option to store) the points of raw mapping to real, since then one could swap out fit functions, but here the coefficients from the fit look to be what is in the config file, if I'm reading it right. Any comment?

That's a good idea. I think it would have to do both though, allow the calibrator to store the raw data (measuredData as they ref to it) which could be null but also allow for the coeffs to be explicitly passed to the transformers - along with some logic/flags to account for the presence of both and exceptions etc. The only issue here is that I don't have any measuredData for temp, just the coeffs in https://raw.githubusercontent.com/FYNCH-BIO/evolver/master/evolver/calibrations.json. But that can be added later I guess.

amitschang · 2024-09-03T19:23:20Z

evolver/hardware/standard/temperature.py

+                    if isinstance(self.calibrator.output_transformer, dict):
+                        temperature = self.calibrator.output_transformer[vial].convert_to(raw)
+                    else:
+                        temperature = self.calibrator.output_transformer.convert_to(raw)
+


Sorry @jamienoss , I should have just put it here. I mean that maybe we don't need to bother with the non-vial case, the procedure really should be producing one per vial even if there is only one vial

Cool, thanks. I re-read your comment and realised what you were referring to.

I thought it was more useful and extensible to allow for both. I've also factored this out to the hardware interface as:

def _transform(self, transformer, func, x, vial=None): if self.calibrator and (_transformer := getattr(self.calibrator, transformer, None)): if isinstance(_transformer, dict): y = getattr(_transformer[vial], func)(x) else: y = getattr(_transformer, func)(x) else: y = x return y

to reduce boilerplate.

👍 ah, ok this will simplify while also being flexible

jamienoss · 2024-09-04T18:50:18Z

@amitschang FYI I'm gonna punt Calibrator.Config.measured_data (or its similar implementation) to #165. This way I can handle the issue of the coeffs being required by the transformers later and get all of the other hardware transformers merged instead.

amitschang

Looks good, just a couple small comments.

amitschang · 2024-09-05T20:20:37Z

evolver/hardware/interface.py

@@ -25,6 +26,18 @@ def __init__(self, *args, evolver=None, **kwargs):
        self.evolver = evolver
        super().__init__(*args, **kwargs)

+    def _transform(self, transformer: str, func: str, x: Any, vial: int = None):


I wonder if it might be more general if we don't call the func but rather return a transformer, and provide a signature more like transformer_in(self, vial) -> Transformer (effectors) and transformer_out(self, vial) -> Transformer (sensors), called like:

temp = self.transformer_out(vial).convert_to(raw)

it can be reused a bit easer since one could stash the transformer to make further calls if needed. It reads slightly more intentional (arguably)

Could do, but I didn't necessarily want to add it to the hardware interface, which the above suggestion would. There'd then need to be two new funcs: transformer_in and transformer_out (which I think would be better suited as properties), however, this would still leave most of the boilerplate as they could both return None and the caller would still have to check for this before calling, e.g., transformer.convert_to, and handle the noop. So it would read:

if transformer:=self.transform_in(vial): temp = transformer.convert_to(raw) else: temp = raw

or

temp = self.transformer_in(vial).convert_to(raw) if self.transformer_in(vial) else raw

Thoughts?

amitschang · 2024-09-05T20:22:54Z

evolver/tests/conftest.py

+
+
+@pytest.fixture
+def tmp_calibration_dir(monkeypatch, tmp_path):


might be useful as an autouse?

Actually, this doesn't work as the fixture order matters.

Signed-off-by: James Noss <jnoss@jhu.edu>

amitschang

sounds good, thanks!

jamienoss added the calibration label Aug 29, 2024

jamienoss self-assigned this Aug 29, 2024

jamienoss commented Aug 29, 2024

View reviewed changes

jamienoss force-pushed the 147-call-transformers-for-temp branch from 7d8a971 to e7d7b38 Compare August 29, 2024 19:03

jamienoss commented Aug 29, 2024

View reviewed changes

evolver/calibration/standard/polyfit.py

Comment on lines +59 to +60

if save:

calibration_data[transformer].save()

Copy link

Member Author

jamienoss Aug 29, 2024

Choose a reason for hiding this comment

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

smuggle

jamienoss commented Aug 29, 2024

View reviewed changes

amitschang reviewed Sep 3, 2024

View reviewed changes

jamienoss force-pushed the 147-call-transformers-for-temp branch 2 times, most recently from a9599a6 to 411ecce Compare September 4, 2024 18:37

jamienoss mentioned this pull request Sep 4, 2024

Store measured data from calibration in Calibrator.Config #165

Open

jamienoss marked this pull request as ready for review September 4, 2024 19:12

jamienoss requested a review from a team as a code owner September 4, 2024 19:12

jamienoss requested a review from amitschang September 4, 2024 19:12

amitschang reviewed Sep 5, 2024

View reviewed changes

jamienoss added 2 commits September 9, 2024 12:07

147-call-transformers-for-temp-calibration

badcb5b

Signed-off-by: James Noss <jnoss@jhu.edu>

Revert Calibrator.Config.measured_data - punt to #165

9a83a81

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss force-pushed the 147-call-transformers-for-temp branch from a830528 to 9a83a81 Compare September 9, 2024 16:07

jamienoss requested a review from amitschang September 9, 2024 16:23

amitschang approved these changes Sep 9, 2024

View reviewed changes

jamienoss merged commit 3e46745 into main Sep 9, 2024
8 checks passed

jamienoss deleted the 147-call-transformers-for-temp branch September 9, 2024 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

147 call transformers for temp #157

147 call transformers for temp #157

jamienoss commented Aug 29, 2024 •

edited

Loading

jamienoss Aug 29, 2024

jamienoss Aug 29, 2024

jamienoss commented Aug 29, 2024

jamienoss Aug 29, 2024

jamienoss Aug 29, 2024

jamienoss Aug 29, 2024

codecov bot commented Aug 29, 2024 •

edited

Loading

jamienoss Aug 29, 2024 •

edited

Loading

amitschang commented Sep 3, 2024

jamienoss commented Sep 3, 2024 •

edited

Loading

amitschang Sep 3, 2024

jamienoss Sep 3, 2024 •

edited

Loading

amitschang Sep 3, 2024

jamienoss commented Sep 4, 2024

amitschang left a comment

amitschang Sep 5, 2024

jamienoss Sep 9, 2024

amitschang Sep 5, 2024

jamienoss Sep 9, 2024

amitschang left a comment

		input_transformer: dict[int, ConfigDescriptor \| LinearTransformer \| None] \| None = None
		output_transformer: dict[int, ConfigDescriptor \| LinearTransformer \| None] \| None = None



		@pytest.fixture
		def tmp_calibration_dir(monkeypatch, tmp_path):

147 call transformers for temp #157

147 call transformers for temp #157

Conversation

jamienoss commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienoss commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

jamienoss Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

amitschang commented Sep 3, 2024

jamienoss commented Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

jamienoss Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienoss commented Sep 4, 2024

amitschang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitschang left a comment

Choose a reason for hiding this comment

jamienoss commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

jamienoss Aug 29, 2024 •

edited

Loading

jamienoss commented Sep 3, 2024 •

edited

Loading

jamienoss Sep 3, 2024 •

edited

Loading