-
Notifications
You must be signed in to change notification settings - Fork 79
Add sequence parameter #866
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
base: dev/candidates
Are you sure you want to change the base?
Changes from all commits
89d8217
8282f14
d5d07c2
90367e6
6eea423
27c756d
5075575
2755385
dbd1f6d
925db4f
192437d
94f7341
ed6e06d
f033ced
0254aed
49d5e4a
3d5621a
c34a9be
364b8d5
8c2db01
62e19df
6ab4dd4
03b7310
cb07cd6
5971a3b
34c2444
6bf02d3
fc24179
d029850
604845c
3c2c46f
d2023ae
d5c805d
71ce4a7
8e1925e
ad6d27a
c98fbd0
d8c6e11
44180a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |
| - Optional/secondary fields of discrete parameter classes are now keyword-only | ||
|
|
||
| ### Added | ||
| - `SequenceParameter` class for modeling parameters whose values are configurable-length | ||
| token sequences from a predefined alphabet | ||
|
Comment on lines
+25
to
+26
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add here: linear & grammar free? |
||
| - `EncoderProtocol` as a public interface for specifying parameter encoders | ||
| - `coefficients` attribute for `DiscreteSumConstraint`, enabling weighted sums. Follows | ||
| the same pattern as `ContinuousLinearConstraint.coefficients` | ||
| - `simplex_coefficients` keyword argument to `SubspaceDiscrete.from_simplex` for | ||
|
|
@@ -30,7 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |
| - `CandidatesProtocol` as an interface for candidates generation | ||
| - `EmptyCandidates`, `TableCandidates` and `ProductCandidates` classes implementing | ||
| `CandidatesProtocol` | ||
| - `DiscreteParameter.is_finite` property | ||
| - `DiscreteParameter.__len__` and `DiscreteParameter.is_finite` property | ||
| - `SubspaceDiscrete.batch_constraints` field for storing batch-level constraints | ||
| - `SubspaceDiscrete.from_dataframe` now accepts `batch_constraints` | ||
| - `validate_parameter_input` now accepts an `allow_empty` flag to permit zero-row input | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| from narwhals.stable.v2.dependencies import is_into_series | ||
| from typing_extensions import override | ||
|
|
||
| from baybe.exceptions import InfiniteSpaceError | ||
| from baybe.serialization import ( | ||
| SerialMixin, | ||
| ) | ||
|
|
@@ -131,11 +132,18 @@ class DiscreteParameter(Parameter, ABC): | |
| def values(self) -> tuple: | ||
| """The values the parameter can take.""" | ||
|
|
||
| def __len__(self) -> int: | ||
| """Return the number of values the parameter can take.""" | ||
|
AVHopp marked this conversation as resolved.
|
||
| return len(self.values) | ||
|
|
||
| @property | ||
| def is_finite(self) -> bool: | ||
| """Indicates whether the parameter has a finite number of values.""" | ||
| len(self.values) # <-- raises an error if the parameter is infinite | ||
| return True | ||
| try: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am confused by this. According to the typing of the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly, Raising an error in a property is certainly not 100% nice and we may want to adjust this in the future. However, without a refactoring, this is what we can do, so I simply comply with the existing protocols
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this was meant as a functionality test to avoid making mistakes during development. Since the sequence parameter overrides and uses
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fabianliebig: yeah, you are right, I did not properly think this through. So we have three options:
What do you think?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I would prefer having an explicit assert, returning true and keeping the override. By that we avoid three things:
What do you think? Does that make sense to you?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So basically the version of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably, would most appreciate a real assert actually but as we discuss that previously, the base version is also fine for me 🙃
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you maybe just
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I did not phrase that really well. Ideally I would like to have an assert like |
||
| len(self) | ||
| return True | ||
| except InfiniteSpaceError: | ||
| return False | ||
|
|
||
| @property | ||
| def active_values(self) -> tuple: | ||
|
|
@@ -242,7 +250,7 @@ def summary(self) -> dict: | |
| return dict( | ||
| Name=self.name, | ||
| Type=self.__class__.__name__, | ||
| nValues=len(self.values), | ||
| nValues=len(self), | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -261,7 +269,7 @@ class _EncodedDiscreteParameter(DiscreteParameter, ABC): | |
| _active_values: tuple[str | bool, ...] | None = field( | ||
| default=None, | ||
| converter=optional_c( | ||
| Converter( # type: ignore[misc, call-overload] | ||
| Converter( # type: ignore[misc] | ||
| nonstring_to_tuple, takes_self=True, takes_field=True | ||
| ) | ||
| ), | ||
|
|
@@ -301,10 +309,10 @@ def _validate_active_values( # noqa: DOC101, DOC103 | |
| ) | ||
| if len(set(content)) != len(content): | ||
| raise ValueError("The active parameter values must be unique.") | ||
| if not all(v in self.values for v in content): | ||
| if invalid := [v for v in content if not self.is_in_range(v)]: | ||
| raise ValueError( | ||
| f"All active values must be valid parameter choices from: " | ||
| f"{self.values}, provided: {content}" | ||
| f"All active values must be valid parameter choices. " | ||
| f"Provided invalid values: {invalid}" | ||
| ) | ||
|
|
||
| @override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| """Sequence encoders.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import gc | ||
| from typing import TYPE_CHECKING, Protocol, runtime_checkable | ||
|
|
||
| import narwhals.stable.v2 as nw | ||
| from attrs import define, field | ||
| from attrs.validators import instance_of | ||
| from exceptiongroup import ExceptionGroup | ||
|
|
||
| from baybe.settings import active_settings | ||
| from baybe.utils.dataframe import _df_with_backend | ||
|
|
||
| if TYPE_CHECKING: | ||
| from narwhals.stable.v2.typing import IntoDataFrame, IntoSeries | ||
|
|
||
|
|
||
| @runtime_checkable | ||
| class EncoderProtocol(Protocol): | ||
|
AVHopp marked this conversation as resolved.
|
||
| """Type protocol specifying the interface encoders need to implement.""" | ||
|
|
||
| # Use slots so that derived classes also remain slotted | ||
| # See also: https://www.attrs.org/en/stable/glossary.html#term-slotted-classes | ||
| __slots__ = () | ||
|
AVHopp marked this conversation as resolved.
|
||
|
|
||
| def __call__(self, series: IntoSeries, /) -> IntoDataFrame: | ||
| """Encode a given series of values. | ||
|
|
||
| Args: | ||
| series: A series in an arbitrary backend, containing the values to encode. | ||
|
|
||
| Returns: | ||
| A dataframe containing the encoded representations of the input values, in | ||
| the same backend as the input series and with the same row order. | ||
|
AVHopp marked this conversation as resolved.
|
||
| """ | ||
|
|
||
|
|
||
| @define | ||
| class _Encoder: | ||
| """A narwhals wrapper for user-specified encoders to hide their native backend. | ||
|
|
||
| Wraps a user-provided instance of an :class:`EncoderProtocol` and automatically | ||
| infers which native dataframe backend it expects via trial and error. The inferred | ||
| backend is cached after the first successful call so that trial-and-error detection | ||
| runs only once. | ||
| """ | ||
|
|
||
| _encoder: EncoderProtocol = field( | ||
| alias="encoder", validator=instance_of(EncoderProtocol) | ||
| ) | ||
| """The user-provided encoder.""" | ||
|
|
||
| _implementation: nw.Implementation | None = field( | ||
| default=None, init=False, eq=False | ||
| ) | ||
| """The inferred native backend, cached after the first successful call.""" | ||
|
|
||
| def __call__(self, series: nw.Series, /) -> nw.DataFrame: | ||
| """Encode a narwhals series, inferring the required backend if not yet known. | ||
|
|
||
| On the first call, tries available backends in order (starting with | ||
| :attr:`~baybe.settings.Settings.default_dataframe_backend`) until the wrapped | ||
| callable succeeds. The successful backend is cached for all subsequent calls. | ||
|
|
||
| Args: | ||
| series: A narwhals series containing the values to encode. | ||
|
|
||
| Returns: | ||
| A narwhals dataframe containing the encoded representations, collected into | ||
| the same backend used for the input series. | ||
| """ | ||
| if self._implementation is None: | ||
| self._implementation, result = self._infer_backend(series) | ||
| return result | ||
|
|
||
| return self._encode(series, self._implementation) | ||
|
|
||
| def _infer_backend( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on this? The user will need to wirte an Encoder with a call that uses some backend, can't we simply infer which the user wants to use from that? Just trying it out according to an arbitrary ordering feels weird to me.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have a better solution, let me know. The problem is that the user can write their callable using arbitrary logic and the hole point is that we don't want to impose any backend on them. So without "looking into" the blackbox, I see no way how we could possibly know what they used. The situation is essentially the following: def my_encoder(dataframe): # no annotations
# code hiddenThe above is what we get. So with which backend would you now call it?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, we could also have proper typing of the encoder as a requirement. Like "We promise that we can handle all backends, but please tell us which you use", I think this is not too hard of a requirement
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I would also prefer it this way. Automatic backend inference could lead to unforeseen issues, so I would rather make the backend explicit and ensure that users are aware of their responsibility. I could only come up with a toy example, but in the following code the loop detects Polars, since both Polars and pandas share the interface used in the first encoder call. However, it later crashes because Polars class StatefulEncoder:
def __init__(self) -> None:
"""Initialize the call counter."""
self._n_calls = 0
def __call__(self, series):
self._n_calls += 1
if self._n_calls == 1:
return pd.DataFrame({"encoded": list(series)})
return pd.DataFrame({"encoded_upper": series.values.tolist()})
def main() -> None:
s = nw.from_native(pd.Series(["a", "b", "c"], name="x"), series_only=True)
enc = _Encoder(encoder=StatefulEncoder())
result1 = enc(s)
print(f"Detected backend : {enc._implementation}")
print(f"Result 1 columns : {result1.columns}")
print()
result2 = enc(s). #<-- Crash
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a valid point! I think the decision we have to make is here is the balance between precision and convenience:
So one potential middle ground I could see is to simply make both the
What do you think? Implementation-wise, the only difference to the case where we drop the auto-inference completely is having the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that sounds like it also shapes awareness. Seems sufficient for me. I actually don't think that it will be that uncommon for something analogous to my example to appear; the most straightforward counterexample might be stateful, which is why I provided it, but I could also imagine some more complex logic that involves different sources like file systems or databases that are used in combination, just because it offers that freedom. :D |
||
| self, series: nw.Series, / | ||
| ) -> tuple[nw.Implementation, nw.DataFrame]: | ||
| """Infer the encoder's backend by trial-and-error across available backends. | ||
|
|
||
| Args: | ||
| series: The series to use for probing. | ||
|
|
||
| Returns: | ||
| A tuple containing: | ||
|
|
||
| * the first backend for which the encoder succeeds | ||
| * the resulting encoded dataframe | ||
|
|
||
| Raises: | ||
| ExceptionGroup: If the encoder raises an exception for every tried backend. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Statement is unclear, sounds like it raises all exceptions every time, but it is just in the case that no suitable backend is found. |
||
| """ | ||
| preferred = active_settings.default_dataframe_backend | ||
| ordered = [preferred] + [b for b in nw.Implementation if b is not preferred] | ||
| backends = [b for b in ordered if _is_backend_imported(b)] | ||
|
|
||
| exceptions: list[Exception] = [] | ||
| for backend in backends: | ||
| try: | ||
| result = self._encode(series, backend) | ||
| return backend, result | ||
| except Exception as ex: # noqa: BLE001 | ||
| exceptions.append(ex) | ||
|
|
||
| raise ExceptionGroup("The encoder failed for all tried backends", exceptions) | ||
|
|
||
| def _encode(self, series: nw.Series, backend: nw.Implementation, /) -> nw.DataFrame: | ||
| """Call the encoder with the given series using the specified backend. | ||
|
|
||
| Args: | ||
| series: The series to encode. | ||
| backend: The native backend to convert the series to before encoding. | ||
|
|
||
| Returns: | ||
| A narwhals dataframe in the original series backend. | ||
| """ | ||
| native_series = _df_with_backend(series, backend).to_native() | ||
| result = nw.from_native(self._encoder(native_series), eager_only=True) | ||
| return _df_with_backend(result, series.implementation) | ||
|
|
||
|
|
||
| def _is_backend_imported(backend: nw.Implementation) -> bool: | ||
| """Check if the given native backend has already been imported.""" | ||
| getter = getattr(nw.dependencies, f"get_{backend.value}", None) | ||
| return getter is not None and getter() is not None | ||
|
|
||
|
|
||
| # Collect leftover original slotted classes processed by `attrs.define` | ||
| gc.collect() | ||
Uh oh!
There was an error while loading. Please reload this page.