Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Add MDD method #263

Merged
merged 36 commits into from
Nov 4, 2024
Merged

Conversation

ambroiseodt
Copy link
Contributor

@ambroiseodt ambroiseodt commented Oct 24, 2024

Implement the MDD method from https://arxiv.org/pdf/1904.05801.

Related issue: #254

Core implementation

  • Add MDDLoss and MDD in skada/deep/_adversarial.py
  • Add test_mdd.py in skada/deep/tests/test_deep_adversarial.py
  • Update domain_classifier to deal with multiclass in skada/deep/modules.py

Documentation

  • Update accordingly skada/deep/__init__.py
  • Update accordingly README.md
  • Update accordingly docs in docs/source/all.rst

@ambroiseodt ambroiseodt changed the title [WIP] Add MDD method (#254) [WIP] Add MDD method Oct 24, 2024
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.98%. Comparing base (f4f68b1) to head (2de5598).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
- Coverage   97.20%   96.98%   -0.22%     
==========================================
  Files          61       61              
  Lines        6334     6372      +38     
==========================================
+ Hits         6157     6180      +23     
- Misses        177      192      +15     

@ambroiseodt ambroiseodt changed the title [WIP] Add MDD method [TO REVIEW] Add MDD method Oct 30, 2024
raise ValueError(
"If domain_classifier is None, num_features has to be provided"
)
domain_classifier = DomainClassifier(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we use here the domain Classifier or if we want a DiscClassifier maybe the same that is in the paper

Copy link
Contributor Author

@ambroiseodt ambroiseodt Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed all the nomenclature "domain" to "disc". In the paper, the disc_classifier must have the same architecture as the task classifier. For the moment, I propose to keep domain_classifier and then do another PR to the adversarial modules from the DomainClassifier (call DomainClassifier or DiscClassifier only in the loss definitions like DANNLoss, MDDLoss instead of the modules like DANN and MDD). It may also be needed to update the DomainAwareNet to change the "domain_classifier" to "disc_classifier".

@antoinedemathelin
Copy link
Contributor

It is almost ok for me
Just two little comments

@antoinedemathelin
Copy link
Contributor

Sounds good to me!

@antoinecollas antoinecollas changed the title [TO REVIEW] Add MDD method [MRG] Add MDD method Nov 4, 2024
@antoinecollas antoinecollas merged commit 0a911a9 into scikit-adaptation:main Nov 4, 2024
7 checks passed
@ambroiseodt ambroiseodt deleted the add_mdd branch January 8, 2025 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants