scikit-adaptation / skada

Domain adaptation toolbox compatible with scikit-learn and pytorch
https://scikit-adaptation.github.io/
BSD 3-Clause "New" or "Revised" License
60 stars 16 forks source link

[WIP] Update test suite for base selector functionality #74

Closed kachayev closed 8 months ago

kachayev commented 8 months ago

Notable changes:

kachayev commented 8 months ago

@YanisLalou I have a couple of questions regarding this suite,

  1. test_base_selector_remove_masked_transform has a comment regarding the essence of the check, but I'm not sure the code actually does what is described in there. Could you please double check?
  2. test_base_selector_domains only performs the assert of 'internal' state of the selector, which is something that is better to be avoided in tests. It's better to test the behavior rather than implementation details as the latter might change rather frequently while we only care about behavior staying consistent. What is the observed behavior that is verified in this test?
codecov[bot] commented 8 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8e8fc7b) 87.16% compared to head (434a05b) 87.29%.

Files Patch % Lines
skada/base.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #74 +/- ## ========================================== + Coverage 87.16% 87.29% +0.13% ========================================== Files 38 38 Lines 2439 2433 -6 ========================================== - Hits 2126 2124 -2 + Misses 313 309 -4 ```

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

YanisLalou commented 8 months ago

@YanisLalou I have a couple of questions regarding this suite,

  1. test_base_selector_remove_masked_transform has a comment regarding the essence of the check, but I'm not sure the code actually does what is described in there. Could you please double check?
  2. test_base_selector_domains only performs the assert of 'internal' state of the selector, which is something that is better to be avoided in tests. It's better to test the behavior rather than implementation details as the latter might change rather frequently while we only care about behavior staying consistent. What is the observed behavior that is verified in this test?
  1. For the test_base_selector_remove_masked_transform, we want to test if the _remove_masked behaves as expected for a pipeline with a predictor at the end (so with a .predict() method) and for a pipeline with an adapter at the end (so with a .adapt() method). By just doing a .fit() we can know if _remove_masked behaves as expected because otherwise a ValueError would be raised somewhere down the line in the adapt() and fit() methods respectively. But yes by looking back at the code, we could also add an assert to check if _remove_masked() outputs arrays with coherent shapes (like for test_base_selector_remove_masked_continuous)

  2. Tbh for test_base_selector_domains, the goal was to make codevoc happy. So basically testing if a selector works if we give to it a sample_domain or not. And also to check if the default assigned domains are correct or not. But yeah if its better to not performs asserts of 'internal' state of a selector, I don't really see where we could check that the domains are correctly assigned by default.

kachayev commented 8 months ago

Got it. I'll see what could be done

kachayev commented 8 months ago

Looking closely into domains_, it doesn't have any specific 'publicly visible' behavior because the functionality it was supposed to cover is just not finished. I created a separate issue for this #84.