[MRG] Fix label-aware cost correction in ot.da (closes #664)#833
Conversation
the cost correction that pushes apart differently-labeled samples was computed backwards. it used missing_ys = (ys == -1) and the product of the two missing masks, so the large cost was applied only where both labels are missing, and never where two labeled samples have different labels. with all labels known the correction was a no-op, so ys/yt had no effect on the transport (see PythonOT#664). switched to present masks (ys != -1) so the correction applies exactly to labeled source/target pairs whose labels differ, matching the original pre-vectorized loop. restored the semisupervised tests that had been flipped to assert the buggy no-op (n_unsup == n_semisup) back to asserting the cost actually changes, and added a regression test. closes PythonOT#664
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #833 +/- ##
=======================================
Coverage 96.85% 96.85%
=======================================
Files 128 128
Lines 25659 25683 +24
=======================================
+ Hits 24852 24876 +24
Misses 807 807 🚀 New features to boost your workflow:
|
|
hello @CodingSelim the PR is looking good, what you coded makes sens to me. You need to skip the tf backend for the DA class. @kachayev i'm open to your opinion on this too. I would like to do a release shortly |
|
@rflamary Looks good to me! Left a couple of minor comments/suggestions. |
|
thanks for the review! pushed the MISSING_LABEL constant. on the cost_correction refactor, i think the item assignment (cost_correction[mask] = self.limit_max) won't work across backends, jax arrays are immutable and the tf tensor path doesn't support that kind of in-place scatter either, which is why the original avoids indexing and does it with arithmetic. i kept the multiply approach but it should be fine, the only reason for the catch_warnings is the 0*inf case when limit_max is left at the default inf, and we already scrub those with nan_to_num right after. happy to try a where-based version if you'd prefer that over the warnings filter, that one stays backend safe. |
|
Yep jax is a pain for generic backends... I think the PR looks great, let us wait for tetss to pass and merge it |
|
@CodingSelim you are right! don't worry, it's a chunk of old code anyways |
rflamary
changed the title
…ythonOT#833) * fix label-aware cost correction in ot.da the cost correction that pushes apart differently-labeled samples was computed backwards. it used missing_ys = (ys == -1) and the product of the two missing masks, so the large cost was applied only where both labels are missing, and never where two labeled samples have different labels. with all labels known the correction was a no-op, so ys/yt had no effect on the transport (see PythonOT#664). switched to present masks (ys != -1) so the correction applies exactly to labeled source/target pairs whose labels differ, matching the original pre-vectorized loop. restored the semisupervised tests that had been flipped to assert the buggy no-op (n_unsup == n_semisup) back to asserting the cost actually changes, and added a regression test. closes PythonOT#664 * add PR number to releases entry * Apply suggestion from @rflamary * use MISSING_LABEL constant instead of bare -1 --------- Co-authored-by: Rémi Flamary <remi.flamary@gmail.com>
3 participants
Types of changes
Motivation and context / Related issue
Closes #664.
The cost correction in
BaseTransport.fitthat is supposed to push apart source and target samples with different labels was computed backwards. It builtmissing_ys = (ys == -1)/missing_yt = (yt == -1)and used their outer productmissing_labels, so the largelimit_maxcost was applied only where both labels are missing, andlabel_matchwas actually a mismatch mask. The net effect:missing_labelsis all zeros) the correction is a complete no-op, soys/ythad no influence on the transport at allThis is why a supervised fit gave the same cost matrix as an unsupervised one.
Fix: use present masks
(ys != -1)/(yt != -1), so the correction applies exactly to labeled source/target pairs whose labels differ. This matches the original pre-vectorized loop (thefor c in classes: cost_[idx_s, idx_t] = limit_maxversion) that the vectorization in #587 replaced.How has this been tested
test_da.pyhad been flipped when the bug was introduced to assertn_unsup == n_semisup(i.e. that supervised mode does nothing), while the comment right above still said the norms should be different. Restored them to assert the cost actually changes.test_semisupervised_cost_correction: with all labels known, different-label pairs get pushed tolimit_maxand same-label pairs keep their smaller cost; in the semisupervised case, columns with an unlabeled (-1) target are never forbidden. It fails on currentmasterand passes with this change.test_da.pypasses locally.PR checklist