-
Notifications
You must be signed in to change notification settings - Fork 917
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
xai init #909
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool addition to darts! 🚀 Added a few comments, may look at it in more detail later.
It looks like a great start and I really like the result :) I also like the structure with separate explainer objects of different kinds living in the darts.explainability submodule. A couple of points:
|
Codecov ReportBase: 93.98% // Head: 93.82% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #909 +/- ##
==========================================
- Coverage 93.98% 93.82% -0.17%
==========================================
Files 73 78 +5
Lines 8200 8505 +305
==========================================
+ Hits 7707 7980 +273
- Misses 493 525 +32
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great overall @dumjax , nice one!
I have some comments, but mostly unimportant things. The only somewhat important things IMO are
- Adding unit tests
- Fixing the case where no past or future covariates are given
- Improving the docstrings in a few places (I've made small suggestions, but I think we can improve the explanations and formatting in a few places)
- Factoring out the X/y computation as discussed
I'm really looking forward for us to release it 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we're 95% there, but there are still a few important things to adapt and to polish before we can merge.
I've made lots of small comments, mainly about the documentation (pls double-check how it renders in HTML before merging), variable naming (I think target_names
is confusing, we should use target_components
) and also having the horizons start at 1 instead of 0 to be consistent with our forecasting models.
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
Co-authored-by: Julien Herzen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks a lot for this!
) = self.model.encoders.encode_train( | ||
target=background_series, | ||
past_covariate=background_past_covariates, | ||
future_covariate=background_future_covariates, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged my PR with encoder updates yesterday :) If you apply the two suggestions (with the other suggestion), everything should be up to date!
) = self.model.encoders.encode_train( | |
target=background_series, | |
past_covariate=background_past_covariates, | |
future_covariate=background_future_covariates, | |
) | |
) = self.generate_fit_encodings( | |
series=background_series, | |
past_covariates=background_past_covariates, | |
future_covariates=background_future_covariates, | |
) |
Explanability: Shap with Regression models #609
Summary
First draw of what should be explainability in darts. We use shap library to explain time series, in the case of RegressionModel.