feat: Add DatetimeOrdinal transformer and unit tests (#818) #874
Conversation
This commit fixes two issues that were causing the CI checks to fail for the DatetimeOrdinal transformer. - Corrected the docstring format in the transform() method to resolve the sphinx-build error. - Removed trailing whitespace from a test file to pass the flake8 style check.
This commit fixes issue that were causing the CI checks to fail for the DatetimeOrdinal transformer. - Removed trailing whitespace from a test file to pass the flake8 style checks.
This commit fixes issue that were causing the CI checks to fail for the DatetimeOrdinal transformer. - Refactored long lines of code to resolve E501 errors reported by flake8.
|
Hi, @solegalli. Can I get a review? |
There was a problem hiding this comment.
Hi @bermuchin
This is a masterpiece. I am so happy with this PR, it's almost ready to merge, I have very little to add. Thank you so much.
I made a few comments on the tests, one is a nitpick, the others are a bit more important: to test the error message. Here's an example of how we do it for other classes:
Going through the code I notice that we are not consistent with how we test errors, but we should move towards testing that the errors match in for all our tests.
Other than that, the last bit missing is adding a user guide, and adding the new class to the readme and the main doc page: https://github.com/feature-engine/feature_engine/blob/main/docs/index.rst
Thanks a lot! I look forward to merging it!
| ) | ||
|
|
||
|
|
||
| def test_datetime_ordinal_no_variables_specified(df_datetime_ordinal): |
There was a problem hiding this comment.
very nitpick: this test could be put together with test_datetime_ordinal_no_start_date and pass the variable names or None with a pytest fixture, because the test and expected results (test logic) are identical.
|
|
||
| def test_datetime_ordinal_missing_values_raise(df_datetime_ordinal_na): | ||
| transformer = DatetimeOrdinal(missing_values="raise") | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
I normally like testing that the error message matches the expected message as well, because sometimes it fails somewhere else and then we can't be sure.
| def test_datetime_ordinal_missing_values_raise(df_datetime_ordinal_na): | ||
| transformer = DatetimeOrdinal(missing_values="raise") | ||
| with pytest.raises(ValueError): | ||
| transformer.fit(df_datetime_ordinal_na) |
There was a problem hiding this comment.
it should also fail in transform, so we could add that test as well.
|
|
||
|
|
||
| def test_datetime_ordinal_invalid_start_date(): | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
I like testing that the error message matches the expected one.
2 participants
Adds the DatetimeOrdinal, a new transformer to convert datetime variables
into their ordinal representation. This allows using date information as a
continuous numerical feature in machine learning models.
The transformer can automatically find datetime variables or work with a
user-defined list. It also provides an option to set a custom
start_dateto calculate ordinals relative to a specific point in time, which can
help in creating more meaningful features.
Closes #818