[ENH] interface ondil OnlineGamlss wrapper #637
Conversation
arnavk23
requested review from
SaiRevanth25,
felipeangelimvieira and
fkiraly
as code owners
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Thanks! I think the package is no longer called rolch but ondil
|
code quality checks are failing, see https://www.sktime.net/en/stable/developer_guide/coding_standards.html |
arnavk23
changed the title
|
@fkiraly made changes according to your review comments. |
|
@fkiraly Fixed the failing lint checks(isort). Please run the workflows again. |
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Nice!
- why are you doing so many
try/exceptin the_fit? These seem unnecessary. You probably generated this via AI, and the AI has no idea about theondilpackage. Please debug the code yourself instead, do not just apply AI mindlessly. - in the API docs this should not go into the reduction docs section, but "online learning".
Removed unused autosummary for MapieRegressor.
Thank you for your review comments! Worked on both, please see. |
|
|
||
| super().__init__() | ||
|
|
||
| def _fit(self, X, y): |
There was a problem hiding this comment.
Add (..., C=None): to override the baseclass method correctly.
There was a problem hiding this comment.
actually, C is not necessary if the regressor does not support censoring. The base class is smart enough :-)
There was a problem hiding this comment.
Good to know. If including C=None isn't too confusing, it might be worth adding as it raises a lot of false-positives in my local type-checker and may do so for other users.
There was a problem hiding this comment.
ah, I see.
FYI, unfortunately this is not possible since originally censoring was not supported (only ordinary probabilistic supervised regression), so a lot of third party code would use _fit(X, y) as signature. Requiring C suddenly would break all this code, including all estimators then present in skpro itself.
Hence the more lenient extension contract - which also means, the same problem would be caused when starting to require C at any point in time, breaking custom extender code.
There was a problem hiding this comment.
From a power user perspective I think this is also a good compromise solution for the long term - since users not interested in censoring do not need to worry about what that C is.
| Distr = getattr(distr_mod, dist) | ||
| # construct args dict using column names | ||
| vals = {c: df.loc[:, [c]].values for c in df.columns} | ||
| return Distr(**vals, index=X.index, columns=self._y_cols) |
There was a problem hiding this comment.
Looks like there is an issue here is the column names are not strings. In my case, I am transforming X and integer column values are used. Therefore some of the keys in vals are ints.
TypeError: keywords must be strings
3 participants
Reference Issues/PRs
Fixes #470
What does this implement/fix? Explain your changes.
Adds support for
ondilonline probabilistic regressors, enablingskprousers to useOnlineGamlsswith the standard skpro API and tooling.Keeps
ondiloptional and minimizes maintenance surface by lazy imports and best-effort API adaptation.Does your contribution introduce a new dependency? If yes, which one?
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Any other comments?
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
skproroot directory (not theCONTRIBUTORS.md). Common badges:code- fixing a bug, or adding code logic.doc- writing or improving documentation or docstrings.bug- reporting or diagnosing a bug (get this pluscodeif you also fixed the bug in the PR).maintenance- CI, test framework, release.See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.Examplessection.python_dependenciestag and ensureddependency isolation, see the estimator dependencies guide.