[FEAT] Add fugw interface #83
Conversation
fmralign/alignment_methods.py
Outdated
| self, | ||
| X, | ||
| Y, | ||
| segmentation, |
There was a problem hiding this comment.
sklearn types estimators tend to mvoe all parameters (beyond X,Y) to the builder and have a very homgeneous fit(X,Y) API. Could we at least get closer to it ?
There was a problem hiding this comment.
We should have a discussion on the API...
fmralign/alignment_methods.py
Outdated
| def transform( | ||
| self, | ||
| X, | ||
| device="auto", |
|
Can you fix the CI ? |
Done! |
fmralign/alignment_methods.py
Outdated
| self, | ||
| X, | ||
| Y, | ||
| segmentation, |
There was a problem hiding this comment.
We should have a discussion on the API...
fmralign/alignment_methods.py
Outdated
| ---------- | ||
| X : ndarray of shape (n_samples, n_features) | ||
| Source features | ||
| id_reg: float, in the [0, 1] interval, defaults to 0 |
There was a problem hiding this comment.
these 2 parameters should be specified at the class creation step I think.
There was a problem hiding this comment.
I agree that currently, it aligns with the FUGW API, where initialization adheres to the BaseMapping class:
class BaseMapping:
def __init__(
self,
alpha=0.5,
rho=1,
eps=1e-2,
reg_mode="joint",
divergence="kl",
):
Adopting a more sklearn-like approach could be advantageous, yet I haven't explored it in detail.
|
OK, after digging into the sklearn API I see what you mean. |
|
We should probably discuss how we move forward with this. This could well be addressed in different PRs. |
|
I'll be out-of-office for a bit this summer, but if the timing lines up -- and if it would be useful to have me participate -- happy to have a discussion on the API ! |
Sure! 🚀 |
The API design has been fixed. |
|
Feel free to merge whenever you want. |
Yes it is 👍 |
I don't have maintainer access, can you merge it ? |
Solves #77 .
Added: