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
[ENH] Refactor of BaseDistribution
and descendants - generalised distribution param broadcasting in base class
#5176
Conversation
Update test class to use the parent _get_bc_params method.
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.
Hm, looks like an impovement!
Left some non-blocking suggestions.
Also, we should see if the tests run - as said, the skpro
tests are more extensive, so let's open a copy there if this runs through.
BaseDistribution
and descendants - generalised distribution param broadcasting in base class
Update distributions to pass tuples to _get_bc_params
Passing a 1D numpy array into the tuple constructor unpacks the elements of the array, we want the whole array to be an item in the tuple.
Update docstring.
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.
Excellent, thanks! This will help greatly with the base class refactor.
…stribution param broadcasting in base class (#54) Mirror of sktime sktime/sktime#5176 Refactors the `_get_bc_params` function in the `BaseDistribution` class. Moves the `_get_bc_params` method from child distributions to the parent distribtion class, `BaseDistribution`. This implementation is quite simple and doesn't use `_tags` and still means the child distributions have to call the `_get_bc_params` method.
Reference Issues/PRs
Related issue sktime/skpro#21
What does this implement/fix? Explain your changes.
Moves the
_get_bc_params
method from child distributions to the parent distribtion class,BaseDistribution
.Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
This implementation is quite simple and doesn't use
_tags
and still means the child distributions have to call the_get_bc_params
method.Would you prefer the child distributions to not have to call the
_get_bc_params
method. This will mean putting the parameter names that need to broadcasted in_tags
dictionary then writing the broadcasted parameters fromBaseDistribution
. Personally I think this is slightly over complicated although this approach follows the DRY principles more closely. I am open to intput here and happy to implement whatever is preferred.Did you add any tests for the change?
Altered the
_DistrDefaultMethodTester
class to use the parent_get_bc_params
method.Any other comments?
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
sktime
root 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 pluscode
if you also fixed the bug in the PR).maintenance
- CI, test framework, release.See here for full badge reference
See here for further details on the algorithm maintainer role.
For new estimators
docs/source/api_reference/taskname.rst
, follow the pattern.Examples
section.python_dependencies
tag and ensureddependency isolation, see the estimator dependencies guide.