eth-easl / modyn

Modyn is a research-platform for training ML models on dynamic datasets.
MIT License
22 stars 3 forks source link

fix: Adjust evaluation business logic #535

Closed robinholzi closed 2 weeks ago

robinholzi commented 2 weeks ago

Motivation

Note

Independently of the bug we fix with the start_timestamp in timetrigger, this setting allows us to effectively do pre-training with the first trigger (e.g. in the continous arxiv dataset we have sparse data from 1988 - ~2005). We could simply start the schedule in year 2005, then the first trigger trains on all previous data.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.46%. Comparing base (cb0be37) to head (e1ee770).

Files Patch % Lines
modyn/supervisor/internal/utils/time_tools.py 0.00% 3 Missing :warning:
modyn/supervisor/internal/triggers/timetrigger.py 77.77% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #535 +/- ## ========================================== - Coverage 82.49% 82.46% -0.04% ========================================== Files 215 216 +1 Lines 10054 10059 +5 ========================================== + Hits 8294 8295 +1 - Misses 1760 1764 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 2 weeks ago

Line Coverage: -% ( % to main) Branch Coverage: -% ( % to main)

MaxiBoether commented 2 weeks ago

i will wait for integrationtests to run through for this one and then merge

robinholzi commented 2 weeks ago

LGTM. I guess in most cases one should set the start timestamp to 0, if they want a neat triggering boundary. But I guess finding the correct setting here is the task of the user.

For continuous datasets with periodic evaluation not necessarily. There you don't really care where it starts and what the exact bounds are, you just care about time proximity. But yes, for the current slicing approaches we need to set it!

robinholzi commented 2 weeks ago

@MaxiBoether could you kick off a cglm pipeline after merging this? I need a pipeline log file with correct training intervals (not affected by the first sample)