Closed fkiraly closed 4 months ago
Hi @fkiraly I am very interested in being assigned to this issue!
My solution to "the class parameterization" is that we store the parameter of the class and then pass it to the methods. I have created a preliminary solution for this. I have compared it to the existing distribution class and it has a similar result
Regarding the "energy and similar integrals are not implemented in scipy", I haven't found any solution for this, do you have any suggestions? Thank you so much!
Yes, this is exactly what I meant - although on an adatper level, rather than for each distribution. That is, inheriting from the adapter and putting the scipy
class in a field should do the job.
Similar to _PytsAdapter
and _TslearnAdapter
in sktime
.
Hi @fkiraly , firstly I want to apologize for my recent absence from SKPro project. I have quite a lot of college assigments and exams previously and unfortunately, I wasn't able to contribute as much as I would have liked 😅. However, currently there is not a lot of things on my way and I am eager to continue contributing 😅
Similar to _PytsAdapter and _TslearnAdapter in sktime.
I will check those two ASAP. Looking forward to catching up and contributing!
What I understand from _PytsAdapter
and _TslearnAdapter
is that we create an Adapter Class that will be a base class for wrapping another library functionality.
We create the base class to have some methods that the child classes can override by returning the specific class/function from the library that we want to wrap
# _PytsAdapter
def _get_pyts_class(self):
raise NotImplementedError("abstract method")
# RocketPyts
def _get_pyts_class(self):
from pyts.transformation.rocket import ROCKET
return ROCKET
We then initialize the class and store it on the attribute. When we want to the methods needed, we use that initialized attribute and use the functionality
# _PytsAdapter
def _predict(self, X, y=None):
pyts_est = getattr(self, self._estimator_attr)
return self._call_with_y_optional(pyts_est.predict, X, y)
What I am confused about is that, the _PytsAdapter
and _TslearnAdapter
initialized the estimator attribute on the _fit
method. But in the scipy
adapter, there is no method bound to be called first (unlike _fit
method). Do we need to move the initialization on the constructor method for scipy
adapter?
I will try to raise a draft PR containing my proposed adapter ASAP
Welcome back, @malikrafsan!
Generally, no need to apologize - as long as you are not paid or have not otherwise committed, there is no expectation that you deliver. Most contributions are on a volunteer or reseach basis, so there is absolutely no pressure. Thanks a lot for your previous work!
Re the adapter, the main idea would be avoiding duplication in classes interfacing scipy
classes - these are already vectorized methods, but do not store parameters. The design principle here is being "DRY" - don't repeat yourself.
The precise way to achieve this depends on the inner and outer interface of the adapter, so pyts
/ tslearn
adapters are not 100% comparable - as you say, there is no fit
in scipy
distributions.
So, one would have to design something where the scipy
distribution, with some information about parameter names, sits inside the adapter.
What tends to be helpful is to interface a few instances and then abstract from them:
Fisk
).Poisson
. This will need to be refactored.My strategy would be: start with Fisk
and try to make it more general. Make a copy and turn it into an adapter class. Then have Fisk
inherit from the adapter, and simplify as much as possible. Then create a second distribution, and then refactor Poisson
.
Hi @fkiraly, thank you so much for your understanding!
I have created a new draft PR containing the proposed base Scipy adapter class and inherited adapter class for the Fisk and Poisson distributions here. Please let me know your thoughts about it, I would be more than happy to get your feedback!
Once again, thank you so much for having me!
Adapting
scipy
distributions is very formulaic and could easily be dealt with by an adapter class. This also avoids duplication in implementation efforts, asscipy
is a core dependency.This may require some abstraction around methods, but it seems mostly like a delegator class, as
scipy
does its own broadcasting.Further discrepancies to be mindful of:
energy
and similar integrals are not implemented inscipy
scipy
uses class methods whereasskpro
uses__init__
based parameterizationGood first issue with a design/architecture flavour, can be leveraged to cover a lot of ground in https://github.com/sktime/skpro/issues/22.