Refactor `Task` class to include multiple rolling window evaluations #33

shchur · 2025-08-20T11:34:02Z

Issue #, if available:

Description of changes:

Remove the TaskGenerator class
Allow including multiple rolling windows in a single Task
Add a EvaluationWindow class that corresponds to a single cutoff in the original dataset

To do:

Update docs
Update implementations of all Task / EvaluationWindow methods
Update unit tests
Update adapters

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

README.md

src/fev/__init__.py

src/fev/benchmark.py

src/fev/task.py

review-notebook-app · 2025-08-22T15:21:55Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

shchur · 2025-08-22T15:22:42Z

src/fev/__about__.py

@@ -1 +1 @@
-__version__ = "0.6.0rc2"
+__version__ = "1.0.0"


I think we'll need to bump the version to v1.0.0 given how breaking the changes are

I have created a new branch pre-v1.0.0. We can merge all PRs before the next release into this branch to avoid making the documentation on main out-of-sync with the latest stable release.

I don't understand why we need to bump it? Are we sure fev is now stable? If not, I'd stick to 0.7 as per semver.

also, is __about__.py standard / meaningfully consumed by setuptools or similar? why not just define in init.py?

Makes sense, I moved this to __init__.py. Using __about__.py has the advantage of being able to check the version without loading the full package, but it's probably not worth it.

src/fev/adapters.py

abdulfatir · 2025-08-25T11:04:09Z

src/fev/adapters.py

    if as_univariate:
-        if univariate_target_column in past.column_names and univariate_target_column != task.target_column:
+        # Raise error if column called `univariate_target_column` already exists and it's not the *only* target column
+        if univariate_target_column in past.column_names and window.target_columns_list != [univariate_target_column]:


Will we hit this often? If yes, maybe it makes sense to change the default to something which would lead to fewer collisions?

We can only hit this if there is a columns called target in the dataset that is not actually the only target column. This looks like a really rare case to me, so I wouldn't worry about too much.

src/fev/adapters.py

src/fev/task.py

canerturkmen

Looking great! Dropping some comments.

docs/02-dataset-format.ipynb

docs/03-tasks-and-benchmarks.ipynb

canerturkmen · 2025-08-26T08:08:25Z

src/fev/task.py

+        data loading.
+    window_step_size : int | str | None
+        Step size between consecutive evaluation windows. Must be an integer if `initial_cutoff` is an integer.
+        Can be an integer or pandas offset string (e.g., 'D', '15min') if `initial_cutoff` is a timestamp.


hmm. if this pandas candy is there, as a user I expect it to be there for horizon as well for consistency?

Making horizon: int | str can be really annoying for the users since then they won't be able to set MyModel(prediction_length=task.horizon) anymore, and will require some complex pandas offset manipulations.

In contrast, I don't think that user will ever want to access task.window_step_size since this parameter just configures which windows will be produced by iter_windows().

canerturkmen · 2025-08-26T08:32:45Z

src/fev/__about__.py

@@ -1 +1 @@
-__version__ = "0.6.0rc2"
+__version__ = "1.0.0"


I don't understand why we need to bump it? Are we sure fev is now stable? If not, I'd stick to 0.7 as per semver.

canerturkmen · 2025-08-26T08:39:11Z

src/fev/adapters.py

        future: datasets.Dataset,
        *,
-        target_column: str | list[str],
+        target_columns_list: list[str],


nit: if at all possible I would avoid type encodings in variables (i.e., ..._list) unless in local variables where they clearly serve to disambiguate.

The name here was chosen to mimic the task property Task.target_columns_list

fev/src/fev/task.py

Lines 727 to 733 in 9a835d0

@property

def target_columns_list(self) -> list[str]:

"""A list including names of all target columns for this task."""

if isinstance(self.target_column, list):

return self.target_column

else:

return [self.target_column]

I think the current naming where target_column can be of type str | list[str] is a bit counterintuitive. How about we rename as follows in a follow-up PR?

Task.target_column -> target: str | list[str]

Task.target_columns_list -> target_columns: list[str]

up to you. I understand the problem, but the solution doesn't help me.

src/fev/task.py

canerturkmen · 2025-08-26T09:03:16Z

src/fev/task.py

+                raise ValueError("`window_step_size` must be an int if `initial_cutoff` is an int")
+            assert self.window_step_size >= 1
+            max_allowed_cutoff = -self.horizon - (self.num_windows - 1) * self.window_step_size
+            if self.initial_cutoff < 0 and self.initial_cutoff > max_allowed_cutoff:


I don't quite follow this check. So if I understand correctly I can only specify initial cutoff to allow for at least the required number of steps (roughly horizon * nr_windows) in the future. if so, why is it not checked for timestamp as well?

In case of timestamp-based cutoffs we cannot perform this check during Task.__post_init__ since we don't yet know which timestamps are available in the dataset.

If there is not enough data to generate the splits with a timestamp-based initial_cutoff, an error will be raised once the user accesses EvaluationWindow.get_input_data() for the window where not enough data is available.

For an integer-based negative initial_cutoff, we can immediately tell if the task configuration is invalid during __post_init__, so this check here is just to save user's time.

src/fev/task.py

test/test_task.py

canerturkmen

Target branch should likely be master. Otherwise, LGTM!

shchur · 2025-08-26T13:43:42Z

Target branch should likely be master. Otherwise, LGTM!

@canerturkmen I will merge pre-v1.0.0 into master after we are done with the refactor to avoid breaking the documentation for the existing users of the latest stable release.

…33)

shchur added 4 commits August 20, 2025 11:12

Refactor the Task object to support multiple rolling windows

472da3b

Update readme

11df3d0

Undo column changes

f1f4383

Fix docstring

980c0ff

shchur requested review from abdulfatir and canerturkmen August 20, 2025 11:38

abdulfatir reviewed Aug 20, 2025

View reviewed changes

README.md Show resolved Hide resolved

src/fev/__init__.py Outdated Show resolved Hide resolved

src/fev/benchmark.py Outdated Show resolved Hide resolved

src/fev/benchmark.py Show resolved Hide resolved

src/fev/task.py Outdated Show resolved Hide resolved

canerturkmen reviewed Aug 20, 2025

View reviewed changes

src/fev/task.py Outdated Show resolved Hide resolved

src/fev/task.py Show resolved Hide resolved

shchur added 3 commits August 20, 2025 14:44

Address PR comments

b3ea978

WIP: update tasks and cutoff handling

eab70bf

Start updating docs

d54cb72

shchur commented Aug 22, 2025

View reviewed changes

shchur added 5 commits August 22, 2025 15:24

Update deprecated args handling

13041cd

Update unit tests

1d0150f

Update quickstart example

032c653

Fix typos in Task docstring

35e0a5a

Update docuemntation

8511d3c

shchur changed the title ~~[WIP] Refactor Task class to include multiple rolling window evaluations~~ Refactor Task class to include multiple rolling window evaluations Aug 25, 2025

shchur changed the base branch from main to pre-v1.0.0 August 25, 2025 10:31

abdulfatir reviewed Aug 25, 2025

View reviewed changes

Address PR comments

9ad0d6f

canerturkmen reviewed Aug 26, 2025

View reviewed changes

shchur added 2 commits August 26, 2025 12:45

Address PR comments

6bfd820

Undo version change

36b3aa8

canerturkmen approved these changes Aug 26, 2025

View reviewed changes

shchur merged commit e308267 into pre-v1.0.0 Aug 26, 2025

shchur deleted the tasks-and-splits branch August 28, 2025 07:18

shchur added a commit that referenced this pull request Sep 16, 2025

Refactor Task class to include multiple rolling window evaluations (#…

14dee1a

…33)

shchur mentioned this pull request Sep 16, 2025

Cherry-pick breaking changes for v0.6.0 into the main branch #46

Merged

	@property
	def target_columns_list(self) -> list[str]:
	"""A list including names of all target columns for this task."""
	if isinstance(self.target_column, list):
	return self.target_column
	else:
	return [self.target_column]

		@@ -1 +1 @@
		__version__ = "0.6.0rc2"
		__version__ = "1.0.0"

		@@ -1 +1 @@
		__version__ = "0.6.0rc2"
		__version__ = "1.0.0"

Refactor Task class to include multiple rolling window evaluations #33

Refactor Task class to include multiple rolling window evaluations #33

Uh oh!

Conversation

shchur commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

review-notebook-app bot commented Aug 22, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

canerturkmen 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!

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!

canerturkmen left a comment

Choose a reason for hiding this comment

Uh oh!

shchur commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Refactor `Task` class to include multiple rolling window evaluations #33

Refactor `Task` class to include multiple rolling window evaluations #33

shchur commented Aug 20, 2025 •

edited

Loading

shchur commented Aug 26, 2025 •

edited

Loading