Adding a periodic encoder to the DatetimeEncoder by rcap107 · Pull Request #1235 · skrub-data/skrub

rcap107 · 2025-02-07T14:35:47Z

Draft for #907

Main points:

Adding transformers for hours in a day and days in a year
Adding the ordinal day (day in a year) as feature
Implement transformers using SplineTransformer from scikit-learn

Questions:

Should I also implement a CircularTransformer?
The main reason I am considering the CircularTransformer is because I am not sure what would be better for encoding days in a year. Should I just not bother, and keep SplineTransformer with a limited number of knots?
What should the default parameters be?
If we decide to encode the year, there is the problem of having both leap and non-leap years in a dataset. Is that a problem? I don't think it would affect the features that much.
Any more features that should be added in this PR?

Of course, tests and examples are all missing.

jeromedockes · 2025-02-07T15:05:20Z

+                self._required_transformers[_case] = _t
+            if _case == "year":
+                # TODO: In theory we should check that the year is leap
+                _t = PeriodicEncoder(kind="circular", period=366)


isn't it a bit surprising that the encoding is different for day and year? maybe it could be splines with 4 splines or something like that?

rcap107 · 2025-02-10T15:48:14Z

I finished the base implementation and split the PeriodicEncoder into SplineEncoder and CircularEncoder

I am working locally on the examples, and now I need to write all the tests

jeromedockes

thanks a lot @rcap107 ! here is a first few comments

jeromedockes · 2025-02-13T14:40:26Z

    "nanosecond",
 ]

+_DEFAULT_ENCODING_PERIODS = {"year": 4, "month": 30, "weekday": 7, "hour": 24}


why 4 for year? shouldn't we set the period to 12 and use the month number or something like that? or set the period to 366 if we use the day of year?

At the beginning I had 365 and the spline as default, but having 365 features for each year was too much, and I didn't have any better idea than setting it to a random number (4 seasons I guess)

Now that circular is the default, we can set it to 366

jeromedockes · 2025-02-13T14:44:22Z

+        Select the strategy used to encode days in a week. By default, use a
+        CircularEncoder with period=7. If None, no encoding is performed.
+
+    hour_encoding : str, :class:``~CircularEncoder``, :class:``~SplineEncoder`` or None, default="circular"


the naming is inconsistent: does it indicate the time unit that gets encoded, or the one that defines the range?
for example here year_encoding encodes the position within the year, so the name corresponds to the range, but hour_encoding encodes the position within the day

I think we should keep the latter option, so hour_encoding will be sin(hour / 24 * 2* pi) whereas day_of_year_encoding will be sin(day_of_year / 366 * 2 * pi)

jeromedockes · 2025-02-13T15:01:24Z

+            _enc_attr = [attr for attr in self.__dict__ if attr.endswith("_encoding")]
+            for _enc_name in _enc_attr:
+                _enc = self.__getattribute__(_enc_name)
+                _enc_case = _enc_name.split("_")[0]


why "case"?

jeromedockes · 2025-02-13T15:02:34Z

+        # parameters
+        if self.add_periodic:
+            _enc_attr = [attr for attr in self.__dict__ if attr.endswith("_encoding")]
+            for _enc_name in _enc_attr:


detail: local variable names don't often start with an underscore. no special reason but it's one more character and I don't see what it adds

jeromedockes · 2025-02-13T15:04:48Z

+        X_out = sbd.concat_horizontal(X_out, *_new_features)
+
+        # Censoring all the null features
+        X_out = sbd.where_row(X_out, self.not_nulls, _null_mask)


I think the circular and spline encoders could do that themselves on the numpy output and we wouldn't need the where_row

this

X_out[~self.not_nulls] = np.nan

works with pandas, but doesn't work with polars, is there another way?

GaelVaroquaux · 2025-02-15T16:47:00Z

The SplineEncoder and the CircularEncoder are not documented in the API page, and thus they don't render correctly in the docs:
https://output.circle-artifacts.com/output/job/cc1b279e-769f-4515-bd08-30d0ebebd0f6/artifacts/0/doc/reference/generated/skrub.DatetimeEncoder.html

GaelVaroquaux · 2025-02-15T17:00:08Z

+        integer.
+    """
+
+    def __init__(self, period, n_splines=None, degree=3):


In scikit-learn design, it is very frowned upon to have a parameter without a default value

I removed it after @jeromedockes suggested that having a default parameter that has nothing to do with the value to encode (e.g., 24 for month or year) would not be useful

I can add it back

yep sorry about that @rcap107 . Indeed I had said there was no sensible default here, but yeah you can add it back

GaelVaroquaux · 2025-02-15T17:03:01Z

        return tags
+
+
+class SplineEncoder(SingleColumnTransformer):


From a design perspective, this class seems to me really like a super thin wrapper on the scikit-learn SplineTransformer.

Would it not be possible to get rid of it, and use the SplineTransformer? I would like to minimize almost-redundant functionnality

Should I just fold the logic of both encoders back in the main DatetimeEncoder then? CircularEncoder is just wrapping a call to np.sin/np.cos

I was thinking that it could be useful to have periodic encoders for non-datetime features, but maybe I'm wrong

GaelVaroquaux · 2025-02-15T17:07:31Z

+        Period to be used as basis of the trigonometric function.
+    """
+
+    def __init__(self, period):


Same comment about scikit-learn really liking defaults. Put 365 for instance

GaelVaroquaux · 2025-02-15T17:17:07Z

I don't understand why, when I look at the generated example gallery, it looks like the relevant examples did not run:

I would really like to see the examples running. I don't have a feeling for merging a PR without the examples running.

GaelVaroquaux · 2025-02-15T17:24:14Z

+
+    year_encoding : str, :class:``~CircularEncoder``, :class:``~SplineEncoder`` or None, default="circular"
+        Select the strategy used to encode days in a year. By default, use a
+        CircularEncoder with period=365. If None, no encoding is performed.


If I follow things right, this is used only if "add_periodic" is True above. We should mention this.

GaelVaroquaux · 2025-02-15T17:24:37Z

+        CircularEncoder with period=365. If None, no encoding is performed.
+
+    month_encoding : str, :class:``~CircularEncoder``, :class:``~SplineEncoder`` or None, default="circular"
+        Select the strategy used to encode days in a month. By default, use a


Same comment here: this is used only if "add_periodic" is True above

GaelVaroquaux · 2025-03-18T17:12:15Z

 from skrub import DatetimeEncoder

-date_enc = DatetimeEncoder().fit_transform(date)
+date_enc = DatetimeEncoder(resolution="hour").fit_transform(date)


Why did we have to add this?

I always try to have something in an example only if it is needed for didactic reasons

To make it clear that's how we can specify the resolution, it's either that or adding a line to the previous paragraph saying that "hour" is the default resolution of the encoder

Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org>

GaelVaroquaux · 2025-03-19T08:44:35Z

To make it clear that's how we can specify the resolution, it's either that or adding a line to the previous paragraph saying that "hour" is the default resolution of the encoder

I would rather have a one-liner comment just above

jeromedockes

this is shaping up very nicely :) I didn't have time to finish the review but I'll get back to it tomorrow

jeromedockes · 2025-03-19T16:18:11Z

    extracted_features_ : list of strings
        The features that are extracted, a subset of ["year", …, "nanosecond",
-        "weekday", "total_seconds"]
+        "weekday", "total_seconds"]. If ``add_periodic=True``, the extracted


I think this part is outdated there is no "add_periodic" anymore

jeromedockes · 2025-03-19T16:59:53Z

the failure is unrelated, I guess the model we were using in a test is not available on huggingface anymore

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

jeromedockes · 2025-03-21T08:45:18Z

+
+Bug fixes
+---------
+- Fixed a bug that caused the :class:`StringEncoder` to not work in presence of null values.


that was a different PR

jeromedockes

very cool @rcap107 , we're down to nitpicks now 🚀

jeromedockes · 2025-03-21T09:11:54Z

+            if self.add_day_of_year:
+                encoding_level = encoding_level + ["day_of_year"]
+            if self.add_weekday:
+                encoding_level += ["weekday"]


could you add a test to cover this line? thanks

jeromedockes · 2025-03-21T09:13:50Z

+        if self.periodic_encoding is not None:
+            encoding_level = list(_DEFAULT_ENCODING_PERIODS.keys())[1 : idx_level + 1]
+            if self.add_day_of_year:
+                encoding_level = encoding_level + ["day_of_year"]


IIUC when we add day of year we get the spline/circular encoding of both the month and the day of year, that's probably redundant right? do you think that day_of_year will be rarely used? or maybe we should always exclude the month and include the day of year in the peridoic encoders?

I don't know what the best solution would be, I'm already encoding the day of year over 12 splines so yes, either one is probably redundant.

I can see day_of_year being used on its own (encoding or not)

yeah, I would say month & day of year once encoded bring the same information (position within the year); I guess day of year in theory has a slightly better resolution (although that difference is probably not visible after smoothing by using only 12 splines). so I would say keep just 1 of the 2, whichever is easiest. and users who want periodic encoding and also the raw day of year are more of a special case I think, so they can use a columntransformer or feature union (for now, soon the expressions API), or we can think of a better way to accomodate that later if needed

jeromedockes · 2025-03-21T09:19:40Z

+
+        self.transformer_ = self._periodic_spline_transformer()
+
+        X_out = self.transformer_.fit_transform(sbd.to_numpy(X).reshape(-1, 1))


I have the impression it would be easier to do the handling of null values here, on the numpy arrays, but maybe I'm wrong

Yes looking at the code you might be right, but I don't think there's a point in moving the check here now that it's being addressed in the main transform

I'll keep it in mind for the next time though

jeromedockes

Looks good ! thanks a lot for all this hard work @rcap107 . this will be a great addition and make the tablevectorizer usable with linear models when there are dates

jeromedockes · 2025-03-21T10:28:33Z

+
+@where_row.specialize("pandas")
+def _where_row_pandas(obj, mask, other):
+    return obj.apply(pd.Series.where, **{"cond": mask, "other": other})


just out of curiosity why not cond=mask, other=other?

Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org> Co-authored-by: Vincent M <maladiere.vincent@yahoo.fr> Co-authored-by: Jérôme Dockès <jerome@dockes.org>

Initial commit for periodic encoder

bc9a5bc

jeromedockes reviewed Feb 7, 2025

View reviewed changes

rcap107 added 4 commits February 10, 2025 16:12

Refactoring code to split Spline and Circular encoders

176b18f

WIP Refactoring code to split Spline and Circular encoders

cc6455c

Renaming a constant

99b2d37

Finished implementation of the circular encoder

ee24931

Updating changelog

1a53da2

Vincent-Maladiere reviewed Feb 11, 2025

View reviewed changes

Comment thread skrub/_datetime_encoder.py Outdated

rcap107 added 6 commits February 11, 2025 11:58

✅ WIP Improving test coverage

156d397

Fixing tests

3004442

Fixing more bugs with testing

9e6e05f

Fixing a typo

542522e

Fixing all tests and docstring shenanigans

3804dc9

Fixing bugs, adding tests, adding docstrings

a981526

rcap107 marked this pull request as ready for review February 13, 2025 10:16

Delete example_circular_encoding.py

3380f83

GaelVaroquaux reviewed Feb 13, 2025

View reviewed changes

Comment thread skrub/_datetime_encoder.py Outdated

Adding doc classes

ed5eb46

jeromedockes reviewed Feb 13, 2025

View reviewed changes

Implementing fixes from comments

ea209d6

GaelVaroquaux reviewed Feb 15, 2025

View reviewed changes

Comment thread skrub/_datetime_encoder.py Outdated

GaelVaroquaux reviewed Feb 15, 2025

View reviewed changes

Comment thread skrub/_datetime_encoder.py Outdated

GaelVaroquaux reviewed Feb 15, 2025

View reviewed changes

GaelVaroquaux reviewed Mar 18, 2025

View reviewed changes

Comment thread examples/03_datetime_encoder.py

GaelVaroquaux reviewed Mar 18, 2025

View reviewed changes

Comment thread examples/03_datetime_encoder.py Outdated

Update examples/03_datetime_encoder.py

801d926

Co-authored-by: Gael Varoquaux <gael.varoquaux@normalesup.org>

rcap107 added 4 commits March 19, 2025 14:30

Addressed a couple comments

e246013

Added a test and fix for a corner case

877efe3

Removing an unneeded test

a3e3edb

Fixing typo in docs

8ad584f

jeromedockes reviewed Mar 19, 2025

View reviewed changes

Comment thread skrub/_datetime_encoder.py Outdated

rcap107 and others added 3 commits March 20, 2025 00:09

Update skrub/_datetime_encoder.py

4405fb2

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

Addressing various comments from the review, fixing tests

1352cbe

Moving all_outputs_ to fit

2c849f8

jeromedockes reviewed Mar 21, 2025

View reviewed changes

rcap107 added 4 commits March 21, 2025 11:12

Addressing more comments, adding more tests

47668ab

reverting changes

d7da4cd

updating changes

79de0a1

reducing number of splines for hour encoding

0fb6cc0

jeromedockes approved these changes Mar 21, 2025

View reviewed changes

fixing some sillyness

ffb5306

rcap107 enabled auto-merge (squash) March 21, 2025 14:19

rcap107 merged commit bf2f396 into skrub-data:main Mar 21, 2025

rcap107 deleted the circular_encoding branch March 21, 2025 14:24

rcap107 mentioned this pull request Mar 24, 2025

FEAT add the Frequency Encoder #707

Closed

jeromedockes mentioned this pull request Apr 7, 2025

Add features to the DatetimeEncoder #907

Closed


		self.transformer_ = self._periodic_spline_transformer()

		X_out = self.transformer_.fit_transform(sbd.to_numpy(X).reshape(-1, 1))

Uh oh!

Conversation

rcap107 commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rcap107 commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GaelVaroquaux commented Feb 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GaelVaroquaux commented Feb 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GaelVaroquaux commented Mar 19, 2025 via email

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rcap107 Mar 21, 2025 •

edited

Loading