Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Finish TODOs in NHiTs and NBEATs #955

Merged
merged 16 commits into from May 18, 2022
Merged

Conversation

gdevos010
Copy link
Contributor

Fixes #.
#954

Summary

Made NHiTs activation configurable between relu and gelu
Apply NHiTs dropout and batch_norm to NBEATs

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #955 (3cc47f1) into master (adb66fd) will increase coverage by 0.00%.
The diff coverage is 93.10%.

❗ Current head 3cc47f1 differs from pull request most recent head e1ed821. Consider uploading reports for the commit e1ed821 to get more accurate results

@@           Coverage Diff           @@
##           master     #955   +/-   ##
=======================================
  Coverage   92.61%   92.61%           
=======================================
  Files          74       74           
  Lines        7404     7423   +19     
=======================================
+ Hits         6857     6875   +18     
- Misses        547      548    +1     
Impacted Files Coverage Δ
darts/models/forecasting/nbeats.py 98.05% <90.47%> (-1.22%) ⬇️
darts/models/forecasting/nhits.py 99.24% <100.00%> (+0.82%) ⬆️
darts/timeseries.py 88.00% <0.00%> (-0.06%) ⬇️
...arts/models/forecasting/torch_forecasting_model.py 89.30% <0.00%> (-0.05%) ⬇️
darts/models/forecasting/forecasting_model.py 96.78% <0.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adb66fd...e1ed821. Read the comment docs.

Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot! I have a couple of minor comments. One of them would be to support a couple more activation functions (the ones in PyTorch that can be easily added). Also how about making the activation configurable in N-BEATS as well, while we're at it? :)

Finally, please add a line to the CHANGELOG.md file listing the changes. You're most welcome to put your name in there too if you want.

darts/models/forecasting/nbeats.py Outdated Show resolved Hide resolved
darts/models/forecasting/nbeats.py Show resolved Hide resolved
darts/models/forecasting/nhits.py Outdated Show resolved Hide resolved
darts/models/forecasting/nhits.py Outdated Show resolved Hide resolved
@gdevos010 gdevos010 requested a review from hrzn May 12, 2022 17:24
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

Thanks, new changes look good. I agree it's probably better to leave batch norm out if it seems to perform badly. Could you maybe add/modify a couple of test cases, to integrate a couple of the new parameters?

darts/models/forecasting/nbeats.py Outdated Show resolved Hide resolved
darts/models/forecasting/nbeats.py Show resolved Hide resolved
darts/models/forecasting/nhits.py Outdated Show resolved Hide resolved
darts/models/forecasting/nhits.py Outdated Show resolved Hide resolved
gdevos010 and others added 4 commits May 15, 2022 17:07
Co-authored-by: Julien Herzen <j.herzen@gmail.com>
Co-authored-by: Julien Herzen <j.herzen@gmail.com>
Co-authored-by: Julien Herzen <j.herzen@gmail.com>
@gdevos010
Copy link
Contributor Author

@hrzn I added an activation test. let me know if I should add any others

@gdevos010
Copy link
Contributor Author

@hrzn Im going to need some help with the failing test. I don't get any errors on my machine.
FAILED darts/tests/models/filtering/test_filters.py::GaussianProcessFilterTestCase::test_gaussian_process

Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

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

Good one, thanks! Will merge.

@hrzn hrzn merged commit 155c653 into unit8co:master May 18, 2022
@gdevos010 gdevos010 deleted the fix/nbeats-nhits-TODOs branch May 27, 2022 19:25
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.

None yet

3 participants