[MRG] Modification source_target_merge function behaviours

scikit-adaptation / skada

Domain adaptation toolbox compatible with scikit-learn and pytorch

https://scikit-adaptation.github.io/

BSD 3-Clause "New" or "Revised" License

56 stars 16 forks source link

[MRG] Modification source_target_merge function behaviours #71

Closed YanisLalou closed 5 months ago

YanisLalou commented 5 months ago

Issue: #61 Change the name of source_target_merge Allow an N dimension array as an input for source_target_merge Add test cases

For source_target_merge:

Sample domain input should be optional
Y_target can be equal to None (check there's only one None given in all pairs)
X_source/target can be list of arrays, check all sizes are coherent

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (52b63b0) 88.54% compared to head (4445f07) 88.83%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #71 +/- ## ========================================== + Coverage 88.54% 88.83% +0.29% ========================================== Files 41 41 Lines 2601 2678 +77 ========================================== + Hits 2303 2379 +76 - Misses 298 299 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

YanisLalou commented 5 months ago

For now source_target_merge() accepts an even number of arrays as input + the sample domain array. It works by considering the arrays by duo (the first one is considered as the source and the second the target). Thus I'm wondering:

if it would be better to accept the duos as tuples rather than directly in the *arrays.
Don't know if the doc is clear enough

Would love your feedbacks

kachayev commented 5 months ago

Regarding pairs, you are right, it might be better with pairs. But it's common to have pairs of ndarrays as an input type... So it might also be a bit confusing. Let's keep the current implementation to see how it works in practice. We will be able to change it if necessary.

kachayev commented 5 months ago

@YanisLalou is this one ready for review?

YanisLalou commented 5 months ago

@YanisLalou is this one ready for review?

Almost, there's one codevoc test that I still need to implement + check if I've managed to handle all possible cases. But otherwise yes you can start reviewing it + if you find any edge cases that I didn't foreseen, that would really help me