Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

[BUG] Fix pyproject package structure and failures in tutorial notebooks #1615

Merged
merged 14 commits into from
Aug 26, 2024
Merged

[BUG] Fix pyproject package structure and failures in tutorial notebooks #1615

merged 14 commits into from
Aug 26, 2024

Conversation

XinyuWuu
Copy link
Member

fix #1539

Removed os.chdir("../../..") in notebooks.

This PR should be merged after #1599

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@XinyuWuu XinyuWuu mentioned this pull request Aug 24, 2024
Merged
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.04%. Comparing base (d82eaaf) to head (a2e8015).
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1615   +/-   ##
=======================================
  Coverage   90.04%   90.04%           
=======================================
  Files          30       30           
  Lines        4722     4722           
=======================================
  Hits         4252     4252           
  Misses        470      470
Flag Coverage Δ
cpu 90.04% <ø> (ø)
pytest 90.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fkiraly fkiraly mentioned this pull request Aug 25, 2024
Merged
fkiraly added a commit that referenced this pull request Aug 25, 2024
@fkiraly
&felipeangelimvieira, yarnabrina [MNT] CI step to test that notebooks…
ccb50ab 
… run (#1621)

This adds a CI step to run all the tutorial notebooks.

The test passes if all tutorial notebooks run without error, and can be used to test the fixes in #1615
@fkiraly fkiraly added the maintenance Continuous integration, unit testing & package distribution label Aug 25, 2024
@fkiraly
Copy link
Collaborator

fkiraly commented Aug 25, 2024

@XinyuWuu, removal of chdir seems to break the imports - very strage, I would have expected them to work due to pip install including the package

@XinyuWuu
Copy link
Member Author

@fkiraly The cause seems to be that pytorch_forecasting is installed as a module instead of a package. Strange.

And build_tools is recognized as package directory by setuptools by default so I have to exclude it.

And quite unintutive,

This wouldn't fine sub packages like pytorch_forecasting.data

[tool.setuptools]
packages = ["pytorch_forecasting"]

fkiraly
fkiraly reviewed Aug 26, 2024
View reviewed changes
docs/source/tutorials/stallion.ipynb
@@ -1917,7 +1917,7 @@
"source": [
"# calcualte metric by which to display\n",
"predictions = best_tft.predict(val_dataloader, return_y=True)\n",
"mean_losses = SMAPE(reduction=\"none\")(predictions.output, predictions.y).mean(1)\n",
"mean_losses = SMAPE(reduction=\"none\").loss(predictions.output, predictions.y[0]).mean(1)\n",
Copy link
Collaborator

@fkiraly fkiraly Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is a workaround for the notebook, but not a fix for #1614, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a workaround.

fkiraly
fkiraly reviewed Aug 26, 2024
View reviewed changes
docs/source/tutorials/ar.ipynb Outdated
"name": "stderr",
"output_type": "stream",
"text": [
"/home/xinyu/DATA1_1T/anaconda3/envs/PF_310/lib/python3.10/site-packages/pytorch_forecasting/models/base_model.py:31: TqdmWarning: IProgress not found. Please update jupyter and ipywidgets. See https://ipywidgets.readthedocs.io/en/stable/user_install.html\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove this warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@fkiraly fkiraly changed the title [BUG] Fix Tutorial Notebooks [BUG] Fix pyproject package structure and failures in tutorial notebooks Aug 26, 2024
@fkiraly fkiraly merged commit 119fa89 into sktime:master Aug 26, 2024
15 checks passed
This was referenced Aug 26, 2024
Open
Open
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@fkiraly fkiraly fkiraly left review comments

Assignees
No one assigned
Labels
maintenance Continuous integration, unit testing & package distribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] SMAPE breaks when executing tutorial
3 participants
@XinyuWuu @codecov-commenter @fkiraly