vshift and hshift for Transformation
#665
Conversation
Turned into a match statement. Also added compressions for transformations which use absolute position if preceded by a pure input shift.
There was a problem hiding this comment.
Pull Request Overview
This PR implements vshift and hshift methods for the Transformation class to enable convenient vertical and horizontal shifting operations. It also enhances transformation compression to handle transformations that use absolute positional arguments preceded by pure input shifts.
- Added
vshiftandhshiftmethods to the baseTransformationclass - Enhanced transformation compression logic to handle positional transformations after pure input shifts
- Added scalar addition and subtraction operators to the
Intervalclass
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| baybe/transformations/base.py | Implements vshift and hshift methods for transformation shifting |
| baybe/transformations/utils.py | Enhances compression logic with pattern matching for positional transformations |
| baybe/utils/interval.py | Adds scalar arithmetic operators for interval shifting |
| tests/test_transformations.py | Adds comprehensive tests for the new shifting functionality |
| CHANGELOG.md | Documents the new features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Scienfitz
force-pushed
the
feature/transformation_shifts
branch
from
d8721b3 to
46ff8eb
Compare
| return self | AbsoluteTransformation() | ||
|
|
||
| def vshift(self, shift: float | int, /) -> Transformation: | ||
| """Add a constant to the transformation (vertical shift).""" |
There was a problem hiding this comment.
Isn't this a technical detail? As a user, I do not care that this technically adds a constant, I only care about the shift. So I would propose "Shift the transformation vertically by a given amount."
There was a problem hiding this comment.
imo this docstring puts both aspects whats done and technically into an extremely short wording. Are you suggesting I need to shorten it further?
There was a problem hiding this comment.
Had the same thought as @AVHopp. Perhaps let's add the "action" first and the (less important) technical explanation second?
| """Add a constant to the transformation (vertical shift).""" | |
| """Shift the transformation vertically (i.e. add a constant to its output).""" |
There was a problem hiding this comment.
since this is the exact opposite of what Alex suggested I will let you both figure it out
for me: its longer and says the same, but it would be nicely consistent between hshift and vshift so imo its acceptable
| return self + shift | ||
|
|
||
| def hshift(self, shift: float | int, /) -> Transformation: | ||
| """Prepend a shift to the input (horizontal shift).""" |
There was a problem hiding this comment.
| """Prepend a shift to the input (horizontal shift).""" | |
| """Shift the transformation horizontally (i.e. add a constant to its input).""" |
There was a problem hiding this comment.
The fact that you wrote add here makes we think maybe we need to discuss this convention here: For hshift I am not adding the constant, I am subtracting it. This amounts to what one would phrase as shift right or shift up on the x axis for a positive number. Do you agree with it?
There was a problem hiding this comment.
Yes, I think your implementation makes sense and is the most intuitive. And then my docstring here would need to be adjusted.
| def test_positional_shift_transformation_compression(t, expected): | ||
| """Simple input shifts are compressed correctly.""" | ||
| t1 = AffineTransformation(shift=-2) | t | ||
| t2 = t.hshift(2) |
There was a problem hiding this comment.
What about tests for vshift?
There was a problem hiding this comment.
vshift has been added to some of the existing tests, see top of the test file changes
|
|
||
| return self | AbsoluteTransformation() | ||
|
|
||
| def vshift(self, shift: float | int, /) -> Transformation: |
There was a problem hiding this comment.
Just wondering if these names are idea. Pro: they are short 👍🏼 But normally, we try to avoid abbreviations and are explicit instead + our methods usually start with verbs. Can we just collect some alternative ideas and then decide if we keep it or not?
Trivial choice:
- shift_horizontally / shift_vertically
There was a problem hiding this comment.
its in accordance with well known hstack and vstack functions + its short. Also what they do is easy to understand so I see 0 reason to lengthen the names (they are probably also commonly used in chaining which suffers visually from long names)
There was a problem hiding this comment.
Ok, if @AVHopp also agrees, let's keep it
| aggregated.append(t) | ||
| last = aggregated.pop() if aggregated else None | ||
| match (last, t): | ||
| case AffineTransformation(), AffineTransformation(): |
There was a problem hiding this comment.
Remember that discussion that I initiated in the Hufhaus when I presented the idea of the new transformation framework? It was about which "standard parameters" we want to include in the base framework and I mentioned that a transformation always consists of three components:
- the actual nonlinearity
- a potential affine input shift
- a potential affine output shift
And we discussed whether it makes sense to include any of the latter two in a "generic form" into the interface. Had we done it, this part wouldn't be necessary, since the code would trivially compress to the two corresponding input shift parameters.
Don't get me wrong: I'm not saying I regret this or we should change it now. But since you seem to see the need for introducing this logic for some reason, I thought I'd point it out again, so that we can think about if a generic solution is preferable?!
There was a problem hiding this comment.
am confused, is this comment on the line or on the overall block?
There was a problem hiding this comment.
if we hade decided otherwise it wouldn't be necessary
-> which means it is currently necessary, because we decided otherwise
Do you not agree with this or what exactly do you want to discuss?
There was a problem hiding this comment.
Not entirely sure myself 😄 I'm trying to recap what exactly we're actually trying to achieve with the compression, and consequently, what should be the right approach.
To clarify why I originally introduced it: equality comparison. If you have something like transformation = AffineTransformation() | IdentityTransformation, this is really same thing as just the identity, but without some additional machinery, we'll never be able to compare that two targets carrying these transforms are the same (same for the transforms themselves).
This raises two questions to me:
- When should compression happen? At the moment when we combine them, which means modifying the objects themselves? Or only for the eq-comparison?
- Who is responsible for triggering the compression? In the current version, it's the part of the code that calls the compression function. Which has the consequence that this function contains the specific logic for all transforms, and when not called, no compression happens. A potential alternative may be to let move the logic to the transforms themselves, into the corresponding dunders. Logic: The triangular transform itself knows best what it means for its parameters when the entire transform is h-shifted.
Any thoughts on this? If we see no clear winner, also don't have to bother at this moment an can refine later when things start to get out of hands. But perhaps this context triggers some thoughts in you.
There was a problem hiding this comment.
so we have the situation A | B and imo a compression should happen whenever A can fully be abstracted into B, which is what happens here:
Ais a pure inputhshiftBis a transformation that stores its definition in terms of absolute positions. Those can be adjusted by whatAshifts, resulting in a newB'
It came up in #657 before we made the clamp changes, there were some equality tests failing of the character described above (The fails are not relevant anymore because the approach was changed, but the compression to me makes sense regardless), indicating that these compressions are missing.
Compression also has a purpose beyond just equality checks: The computation is probably more efficient and less error prone if there are less steps.
Regarding who is responsible, isnt the answer simply ChainedTransformation? The compression is not yet that complicated to warrant more retructuring
3 participants
Implements #651
Also added some things to transformation compression for transformations that use absolute positional arguments of preceeded by a pure input shift