Skip to content

vshift and hshift for Transformation #665

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+92 −18
Open

vshift and hshift for Transformation #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fc5458c
Rework transformation compression
Scienfitz Oct 1, 2025
7ce1958
Add interval arithmetic
Scienfitz Oct 13, 2025
0e4d073
Add methods
Scienfitz Oct 13, 2025
ac110d7
Extend tests
Scienfitz Oct 13, 2025
46ff8eb
Update CHANGELOG.md
Scienfitz Oct 13, 2025
0fd1e19
Include suggestions from code review
Scienfitz Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
set to `True`, targets seek to avoid the given `match_value` instead of matching it.
- Transfer learning regression benchmarks infrastructure for evaluating TL model
performance on regression tasks

- Scalar addition and subtraction for `Interval` objects
- Methods `hshift` and `vshift` to `Transformation` for conveniently performing
horizontal / vertical shifts

## [0.14.1] - 2025-10-01
### Added
- `to_json` and `from_json` methods now also natively support (de)serialization to/from
Expand Down
10 changes: 10 additions & 0 deletions baybe/transformations/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ def abs(self) -> Transformation:

return self | AbsoluteTransformation()

def vshift(self, shift: float | int, /) -> Transformation:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, if @AVHopp also agrees, let's keep it

"""Add a constant to the transformation (vertical shift)."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

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."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

imo this docstring puts both aspects whats done and technically into an extremely short wording. Are you suggesting I need to shorten it further?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Had the same thought as @AVHopp. Perhaps let's add the "action" first and the (less important) technical explanation second?

Suggested change
"""Add a constant to the transformation (vertical shift)."""
"""Shift the transformation vertically (i.e. add a constant to its output)."""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

See other comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""Prepend a shift to the input (horizontal shift)."""
"""Shift the transformation horizontally (i.e. add a constant to its input)."""

Copy link
Collaborator Author

@Scienfitz Scienfitz Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think your implementation makes sense and is the most intuitive. And then my docstring here would need to be adjusted.

from baybe.transformations import AffineTransformation

return AffineTransformation(shift=-shift) | self

def __neg__(self) -> Transformation:
return self.negate()

Expand Down
44 changes: 29 additions & 15 deletions baybe/transformations/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from typing import TYPE_CHECKING

import numpy as np
from attrs import evolve

from baybe.transformations.base import Transformation

Expand Down Expand Up @@ -64,30 +65,43 @@ def compress_transformations(
Returns:
The minimum sequence of transformations that is equivalent to the input.
"""
from baybe.transformations.basic import AffineTransformation, IdentityTransformation
from baybe.transformations.basic import (
AffineTransformation,
BellTransformation,
IdentityTransformation,
TriangularTransformation,
TwoSidedAffineTransformation,
)

aggregated: list[Transformation] = []
last = None
id_ = IdentityTransformation()

for t in _flatten_transformations(transformations):
# Drop identity transformations (and such that are equivalent to it)
if t == id_:
continue

# Combine subsequent affine transformations
if (
aggregated
and isinstance(last := aggregated.pop(), AffineTransformation)
and isinstance(t, AffineTransformation)
):
aggregated.append(combine_affine_transformations(last, t))

# Keep other transformations
else:
if last is not None:
aggregated.append(last)
aggregated.append(t)
last = aggregated.pop() if aggregated else None
match (last, t):
case AffineTransformation(), AffineTransformation():
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. the actual nonlinearity
  2. a potential affine input shift
  3. 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?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

am confused, is this comment on the line or on the overall block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the overall block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. When should compression happen? At the moment when we combine them, which means modifying the objects themselves? Or only for the eq-comparison?
  2. 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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:

  • A is a pure input hshift
  • B is a transformation that stores its definition in terms of absolute positions. Those can be adjusted by what A shifts, resulting in a new B'

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

# Two subsequent affine transformations
aggregated.append(combine_affine_transformations(last, t))
case AffineTransformation(factor=1.0), BellTransformation():
# Bell transformation after a pure input shift
aggregated.append(evolve(t, center=t.center - last.shift))
case AffineTransformation(factor=1.0), TwoSidedAffineTransformation():
# 2-sided affine transformation after a pure input shift
aggregated.append(evolve(t, midpoint=t.midpoint - last.shift))
case AffineTransformation(factor=1.0), TriangularTransformation():
# Triangular transformation after a pure input shift
aggregated.append(
evolve(t, peak=t.peak - last.shift, cutoffs=t.cutoffs - last.shift)
)
case (None, _):
aggregated.append(t)
case (l, _):
aggregated.append(l)
aggregated.append(t)

# Handle edge case when there was only a single identity transformation
if not aggregated:
Expand Down
8 changes: 8 additions & 0 deletions baybe/utils/interval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@ def contains(self, number: float) -> bool:
or (self.lower < number < self.upper)
)

def __add__(self, other: float | int) -> Interval:
"""Shift bounds via scalar addition."""
return Interval(self.lower + other, self.upper + other)

def __sub__(self, other: float | int) -> Interval:
"""Shift bounds via scalar subtraction."""
return self + (-other)


def use_fallback_constructor_hook(value: Any, cls: type[Interval]) -> Interval:
"""Use the singledispatch mechanism as fallback to parse arbitrary input."""
Expand Down
43 changes: 41 additions & 2 deletions tests/test_transformations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,49 @@ def test_affine_transformation_compression():
t = IdentityTransformation()

t1 = t * 2 + 3 | t
assert t1 == AffineTransformation(factor=2, shift=3)
t1b = (t * 2).vshift(3)
assert t1 == t1b == AffineTransformation(factor=2, shift=3)

t2 = (t + 3) * 2 | t
assert t2 == AffineTransformation(factor=2, shift=3, shift_first=True)
t2b = t.vshift(3) * 2
assert t2 == t2b == AffineTransformation(factor=2, shift=3, shift_first=True)


@pytest.mark.parametrize(
"t, expected",
[
param(
BellTransformation(center=0, sigma=1),
BellTransformation(center=2, sigma=1),
id="bell",
),
param(
TwoSidedAffineTransformation(midpoint=0, slope_left=-4, slope_right=2),
TwoSidedAffineTransformation(midpoint=2, slope_left=-4, slope_right=2),
id="2sided_affine",
),
param(
TriangularTransformation(peak=0, cutoffs=(-2, 1)),
TriangularTransformation(peak=2, cutoffs=(0, 3)),
id="triangular_cutoffs",
),
param(
TriangularTransformation.from_margins(peak=0.0, margins=(2, 1)),
TriangularTransformation(peak=2, cutoffs=(0, 3)),
id="triangular_margins",
),
param(
TriangularTransformation.from_width(peak=0.0, width=4),
TriangularTransformation(peak=2, cutoffs=(0, 4)),
id="triangular_width",
),
],
)
def test_positional_shift_transformation_compression(t, expected):
"""Simple input shifts are compressed correctly."""
t1 = AffineTransformation(shift=-2) | t
t2 = t.hshift(2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about tests for vshift?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

vshift has been added to some of the existing tests, see top of the test file changes

assert t1 == t2 == expected


def test_identity_transformation_chaining():
Expand Down