Skip to content

Conversation

mabuimo
Copy link

@mabuimo mabuimo commented Jul 10, 2025

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.
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@benHeid benHeid left a comment

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.

Copy link
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
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
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 <[email protected]>
@mabuimo
Copy link
Author

mabuimo commented Jul 18, 2025

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
Copy link
Author

mabuimo commented Jul 18, 2025

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
Copy link
Author

mabuimo commented Jul 18, 2025

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

Copy link

@fkiraly fkiraly left a comment

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
Copy link
Author

mabuimo commented Aug 4, 2025

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

@fkiraly
Copy link

fkiraly commented Aug 14, 2025

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

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