Conversation
oliwenmandiamond
left a comment
oliwenmandiamond
left a comment
There was a problem hiding this comment.
I think is a good idea, but looks to be overkill. You can simplify this a lot and add it into a utils.py function inside tests/common/general_maths so more helpful functions to assist with testing could be added as well.
| from collections.abc import Callable | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| class FunctionalCircularityTester: | ||
|
|
||
| def __init__(self, forward_func: Callable[[float],float], reverse_func: Callable[[float],float]): | ||
| self.f = forward_func | ||
| self.g = reverse_func | ||
|
|
||
|
|
||
| def verify_forwards_circulation_returns_to_same_value(self, x: float): | ||
| conversion = self.f(x) | ||
| assert self.g(conversion) == pytest.approx(x) | ||
|
|
||
|
|
||
| def verify_reverse_circulation_returns_to_same_value(self, x: float): | ||
| conversion = self.g(x) | ||
| assert self.f(conversion) == pytest.approx(x) |
There was a problem hiding this comment.
I think this is overkill. I think a better solution would be to make it a util function
def assert_roundtrip(forward, reverse, x):
assert reverse(forward(x)) == pytest.approx(x)
assert forward(reverse(x)) == pytest.approx(x)Then you could use it like this:
def test_circular_mm_cm_interconversions():
for x in [1, 2, ...]:
assert_roundtrip(convert_cm_to_mm, convert_mm_to_cm, x)There was a problem hiding this comment.
I don't see why making a class is such a heavy weight cost - there is no over kill.
If everybody in the group had to pay 10 quid every time a class was used then fair.
There was a problem hiding this comment.
also note Your suggested util function is really two functions in one ( but doesn't say that on the tin )
There was a problem hiding this comment.
So your tests are always doing this
t = FunctionalCircularityTester(f, g)
t.verify_forwards_circulation_returns_to_same_value(x)
t.verify_reverse_circulation_returns_to_same_value(x)Why do that when simply do this:
assert_roundtrip(f, g, x)It's a lot more concise and is the point of a util function for testing logic works.
The name can be whatever you want. Maybe assert_forward_reverse_func
There was a problem hiding this comment.
I see Your points and counter with these advantages of the OO assuming immutable classes:
Encapsulation of Behavior: Instead of just grouping data, you are grouping the mathematical contract alongside the functions. The object represents the concept of a "Reciprocal Pair" itself.
Contextual Clarity: In your test file, writing pair = ReciprocalPair(f, g) explicitly names the relationship between f and g. The code reads as: "This object is a Reciprocal Pair, and I am asking it to verify its integrity at point x."
Thread Safety / Zero Side Effects: Because it's immutable, you can instantiate it once at the top of a test module and safely share it across dozens of different test conditions without fear of leaks.
Where the Rub Lies (The "Pythonic" Trade-Off)
The reason many Python developers default to the alternative functional approach isn't because immutability is bad—it’s because of Python's language syntax overhead.
In languages like Scala or Kotlin, immutable classes are incredibly lightweight. In Python, creating a class often requires writing boilerplate (init, type hints, etc.) or importing helper modules (dataclasses, NamedTuple) just to achieve true immutability.
The Verdict under Immutable OO
If your architectural goal is clean, expressive domain modeling where behaviors belong strictly to the nouns they describe, Version 2 is absolutely the better step. It elevates f and g from two random functions passing through a utility pipe into a cohesive, immutable mathematical domain concept.
There was a problem hiding this comment.
I reply to "Why do that when simply do this"
why call "some test line on (x)" <-- one argument
vs
do-everything-in-one-kitchen-sink(f,g,x) <-- three arguments and now the reader has to think about each
There was a problem hiding this comment.
In a perfect world all methods would only ever take zero or one argument
code would be a lot longer
but each line would be very readable
There was a problem hiding this comment.
@oliwenmandiamond: I think Your original points did validly highlight the ambiguities inherent in my earlier stab at this. I've tried to improve the clarity of purpose across the latest update
CoePaul
force-pushed
the
dodal2102
branch
2 times, most recently
from
08a66cb to
38784fd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2106 +/- ##
=======================================
Coverage 99.16% 99.16%
=======================================
Files 347 347
Lines 13585 13585
=======================================
Hits 13471 13471
Misses 114 114 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
also the suggested change is tantamount to where we started |
CoePaul
force-pushed
the
dodal2102
branch
2 times, most recently
from
6cf5d2b to
d71a407
Compare
| @pytest.mark.parametrize( | ||
| "function_pair, numerical_args", | ||
| [ | ||
| ( | ||
| ReciprocalFunctionPair(convert_ev_to_kev, lambda k: k * 1000.0), | ||
| [16.83, 0.0, 0.037, 1.0, 6.208, 18, 12345.6, 28906.4], | ||
| ), | ||
| ( | ||
| ReciprocalFunctionPair(convert_mm_to_cm, convert_cm_to_mm), | ||
| [-16.83, 0.0, 0.037, 1.0, 6.208, 18, 102.99], | ||
| ), | ||
| ( | ||
| ReciprocalFunctionPair( | ||
| convert_microns_to_cm, | ||
| lambda x: convert_cm_to_mm(convert_mm_to_microns(x)), | ||
| ), | ||
| [-6.119, 0.0, 0.764, 1.02, 62.45, 12754, 3154.59], | ||
| ), | ||
| ( | ||
| ReciprocalFunctionPair(convert_microns_to_mm, convert_mm_to_microns), | ||
| [-12.38, 0.0, 0.307, 1.0, 6.45, 24, 231.089], | ||
| ), | ||
| ( | ||
| ReciprocalFunctionPair( | ||
| convert_factor_to_percentage, convert_percentage_to_factor | ||
| ), | ||
| [0.0, 1.0, 0.5, 0.367, 27.404, 100.0, 99.8, 53.647], | ||
| ), | ||
| ], | ||
| ) | ||
| def test_sanity_check_reciprocal_function_pairs( | ||
| function_pair: ReciprocalFunctionPair, numerical_args: list[float] | ||
| ): | ||
| for x in numerical_args: | ||
| assert x == pytest.approx(function_pair.convert_then_invert(x)) | ||
| assert x == pytest.approx(function_pair.invert_then_convert(x)) |
There was a problem hiding this comment.
@pytest.mark.parametrize(
"forward, reverse, numerical_args",
[
(
convert_ev_to_kev,
lambda k: k * 1000.0,
[16.83, 0.0, 0.037, 1.0, 6.208, 18, 12345.6, 28906.4],
),
(
convert_mm_to_cm,
convert_cm_to_mm,
[-16.83, 0.0, 0.037, 1.0, 6.208, 18, 102.99],
),
(
convert_microns_to_cm,
lambda x: convert_cm_to_mm(convert_mm_to_microns(x)),
[-6.119, 0.0, 0.764, 1.02, 62.45, 12754, 3154.59],
),
(
convert_microns_to_mm,
convert_mm_to_microns,
[-12.38, 0.0, 0.307, 1.0, 6.45, 24, 231.089],
),
(
convert_factor_to_percentage,
convert_percentage_to_factor,
[0.0, 1.0, 0.5, 0.367, 27.404, 100.0, 99.8, 53.647],
),
],
)
def test_sanity_check_reciprocal_function_pairs(
forward: Callable[[float], float], reverse: Callable[[float], float], numerical_args: list[float],
):
for x in numerical_args:
assert_forward_reverse(forward, reverse, x)The current ReciprocalFunctionPair implementation stores the two functions and then exposes several wrapper methods that ultimately just call those functions in different orders. Unless we expect to attach additional metadata or behaviour to these pairs in future, I don't see a strong benefit to introducing the extra abstraction.
| class ReciprocalFunctionPair: | ||
| """Ancilliary class which pairs together reciprocal functions for closed-loop testing. | ||
|
|
||
| Converting the numerical part of a cm quantity, for example, into the equivalent numerical part, | ||
| for an inches quantity and then converting back again should result in the number you first started with. | ||
| Such a closed loop is a circularity test. | ||
| Naturally it has two possible directions - so both are covered. | ||
|
|
||
| N.B. Only useful for testing monotonic functions that can only derive an output from one specific input; | ||
| something like squaring and sqrt would not be suitable since -4 -> 16 -> +4 does not close the loop | ||
| back to the original input. | ||
|
|
||
| Attributes: | ||
| forward_func: One of the pair of maths functions, assumed to take a number and return a number. | ||
| inverse_func: The inverse maths functions, assumed to take a number and return a number. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| forward_func: Callable[[float], float], | ||
| inverse_func: Callable[[float], float], | ||
| ): | ||
| """Initializes the FunctionalCircularityTester with a specific pair of functions to test. | ||
|
|
||
| Args: | ||
| forward_func: A unary operator taking a number and returning a number. | ||
| inverse_func: A unary operator taking a number and returning a number. | ||
| """ | ||
| self.f = forward_func | ||
| self.g = inverse_func | ||
|
|
||
| def convert_then_invert(self, x: float): | ||
| """Exercises the circular loop of applying first the forward function then its inverse. | ||
|
|
||
| Args: | ||
| x: a suitable numerical argument which should be compatible with the functions tested. | ||
| """ | ||
| conversion = self._convert(x) | ||
| return self._invert(conversion) | ||
|
|
||
| def invert_then_convert(self, x: float): | ||
| """Exercises the circular loop of applying first the reverse function then the forward. | ||
|
|
||
| Args: | ||
| x: a suitable numerical argument which should be compatible with the functions tested. | ||
| """ | ||
| inversion = self._invert(x) | ||
| return self._convert(inversion) | ||
|
|
||
| def _convert(self, x: float): | ||
| """Performs the forward operation (as defined in the constructor call. | ||
|
|
||
| Args: | ||
| x: a suitable numerical argument which should be compatible with the functions tested. | ||
| """ | ||
| return self.f(x) | ||
|
|
||
| def _invert(self, x: float): | ||
| """Performs the inverse operation (as defined in the constructor call. | ||
|
|
||
| Args: | ||
| x: a suitable numerical argument which should be compatible with the functions tested. | ||
| """ | ||
| return self.g(x) |
There was a problem hiding this comment.
I am not sure how five methods of a class is better than a simple assert function which achieves the same thing.
def assert_forward_inverse(forward: Callable[[float], float], inverse: Callable[[float], float], x: float):
"""Assert that two functions are mutual inverses for a given input.
Verifies that applying the forward function followed by the inverse
returns the original value, and vice versa, within pytest's floating
point tolerance.
Args:
forward: Function mapping a float to a float.
inverse: Function expected to be the inverse of ``forward``.
x: Test value to round-trip through both functions.
"""
assert reverse(forward(x)) == pytest.approx(x)
assert forward(inverse(x)) == pytest.approx(x)I can see the value of the class if we expect to add additional metadata or behaviour to these function pairs in future, but based on the current usage it feels unnecessary.
There was a problem hiding this comment.
- help me get round my mental block:
I'm struggling to avoid reading the review as
"You don't have permission to write declarative code that will aid tests - because
there's no compelling business case for doing so"
There was a problem hiding this comment.
I've requested changes because I agree on the use case but don't agree on the implementation. As this is a general util useful for testing for math function, in theory anyone else should be able to import and use but that isn't possible at the moment. My aim was in the style of ophyd-async assert functions e.g assert_reading, assert_value, assert_configuration, assert_emitted etc with some more examples from dodal.
Thoughts @jacob720 and @rtuck99? I'll let you tip the scales on this PR.
There was a problem hiding this comment.
I originally presented a solution that " anyone else should be able to import and use "
because I widely expect that - especially in green field projects like dodal ( but even secretly in legacy code ).
Once the earlier submissions were shot-down for being "unnecessary because we're not going to use it much"
I took it back out the of the way.
A further update sees the universal accessibility restored.
I do fail to see why using a class is objectionable. OO is a thing - even a good thing.
Use it or lose it.
CoePaul
force-pushed
the
dodal2102
branch
3 times, most recently
from
82a5613 to
3e4d0d5
Compare
* A circularity test acts as a functional sanity check.
Each test acts on pairs of unary operator type functions which reciprocally convert float inputs
(typically for the two directions of quantity conversion between two similar units
Celsius <--> Fahrenheit could hypothetically be a valid example)
* Replace sanity checks that were written from scratch independently
with a set-once use-again, mutually-inverse function pairing
* N.B. The use of a class for the pair rather than a 3 argument util function,
places the two operator functions on a clearly equal footing,
distinct from the arbitarily valued numerical probe argument used in verfication;
whereas a util function muddies the waters, forcing cognitive load back onto the human reader.
Fixes #2102
2 participants
For unary operator functions acting on float inputs like conversions in both directions between one unit and another
This replaces separately rolled once per test circularity with use of a common test circularity framework class
Fixes #2102
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}