Skip to content

[FIX] fh range should begin at 1; dependencies#1

Open
mabuimo wants to merge 5 commits into
sktime:mainfrom
mabuimo:mb-fix
Open

[FIX] fh range should begin at 1; dependencies#1
mabuimo wants to merge 5 commits into
sktime:mainfrom
mabuimo:mb-fix

Conversation

@mabuimo

@mabuimo mabuimo commented Jul 10, 2025

Copy link
Copy Markdown

What does this implement/fix?

  • The range for the fh argument should begin at 1, as 0 would include the last point of the training dataset in the cross-validation.
  • Added uv as an optional dependency manager.
  • The dependencies of requirements.txt were incomplete.

mabuimo added 2 commits July 10, 2025 19:44
fix: if the range starts in 0 it will include the last point of the training set
I added `uv` as an alternative for dependency management. The dependencies from requirements.txt were incomplete. The whole list can be found in the .toml.
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@benHeid benHeid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I have a small suggestion to the code. The only major comment is could you try to rerun notebook 3. It doesn't run completely.

Comment thread .gitignore Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you execute this notebook again and check that Moirai_small runs. The following cell seems to fail

results = benchmark.run("energy_benchmarking.json", force_rerun =True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure, do you think it is useful to push the result file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it is not very useful, we can remove it.

Co-authored-by: Benedikt Heidrich <benHeid@users.noreply.github.com>
@mabuimo

mabuimo commented Jul 18, 2025

Copy link
Copy Markdown
Author

Regarding the lock file, I found this discussion astral-sh/uv#10730 but I agree with you that we don't need to include it.

@mabuimo

mabuimo commented Jul 18, 2025

Copy link
Copy Markdown
Author

Btw @benHeid testing TimeLLMForecast fails with the example of notebook 3.

Code to add to replicate the issue:

from sktime.forecasting.time_llm import TimeLLMForecaster
 benchmark.add_estimator(
     TimeLLMForecaster()
 )

@mabuimo

mabuimo commented Jul 18, 2025

Copy link
Copy Markdown
Author

@benHeid ready for final review. All comments addressed, thank you.

@fkiraly fkiraly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

(@benHeid is currently off sktime, so I am reviewing)

Could you kindly do a clean re-execution of the notebooks? I.e., reset and execute all of them.

@mabuimo

mabuimo commented Aug 4, 2025

Copy link
Copy Markdown
Author

Hi @fkiraly in theory the last PR includes a full re-run of all cells. Isn't that the case? Thanks.

@fkiraly

fkiraly commented Aug 14, 2025

Copy link
Copy Markdown

I think you need to reset the notebook and then re-execute - if the first cell shows execution number 82 and not 1, it has not been reset properly

benchmark.run still fails (!)
@mabuimo

mabuimo commented Oct 18, 2025

Copy link
Copy Markdown
Author

@fkiraly I rerun the notebooks. 01, 02, and 04 notebooks are ok. 03 still fails in the benchmarking module, FYI @jgyasu

RuntimeError: The size of tensor a (71) must match the size of tensor b (70) at non-singleton dimension 3

@fkiraly

fkiraly commented Oct 24, 2025

Copy link
Copy Markdown

I recommend to open an issue on sktime with minimum reproducible code on the remaining issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants