Modality-aware composer #126
Conversation
This reverts commit cb29cde.
mart/attack/composer.py
Outdated
| __all__ = ["Additive", "Overlay", "ModalityComposer"] | ||
|
|
There was a problem hiding this comment.
I don't know if exporting these is smart given their generic names? I wouldn't mind exporting (by default) ModalityComposer though. If they were named AdditiveComposer then the possibility of a naming conflict is low. But mart.attack.Additive feels a little too generic?
mart/attack/composer.py
Outdated
| **kwargs, | ||
| ) -> torch.Tensor | tuple: | ||
| # Bypass batch-aware in Composer.__call__(), because we have the recursive self.compose(). | ||
| input_adv = self.compose(perturbation, input=input, target=target) |
There was a problem hiding this comment.
I would expect, at a minimum, that these arguments (perturbation, input, and target) would have the same types as in compose since you're directly passing them to compose?
My suspicion is that some functionality probably needs to move to Composer.__call__ while dict functionality should be in ModalityComposer.__call__?
There was a problem hiding this comment.
I feel like it will be less confusing if we follow the implementation in Enforcer.
mart/attack/composer.py
Outdated
| class ModalityComposer(Composer): | ||
| """A modality-aware composer. | ||
| Example usage: `ModalityComposer(rgb=Overlay(), depth=Additive())`. Note that | ||
| `ModalityComposer(default=Additive())` is equivalent with `Additive()`. | ||
| """ | ||
|
|
||
| def __init__(self, **modality_composers): | ||
| self.modality_composers = modality_composers | ||
|
|
||
| def __call__( | ||
| self, | ||
| perturbation: torch.Tensor | tuple, | ||
| *, | ||
| input: torch.Tensor | tuple, | ||
| target: torch.Tensor | dict[str, Any] | tuple, | ||
| **kwargs, |
There was a problem hiding this comment.
ModalityComposer feels like it can just wholesale replace Composer? If so, you should just rename this as Composer and get rid of ModalityComposer. The other option is to just have ModalityComposer only take dicts so that it specialized in modalities, which we encode as dicts (as the name suggests).
There was a problem hiding this comment.
I revised the implementation following the structure in Enforcer.
There was a problem hiding this comment.
Maybe we shouldn't merge this PR.
I will try to implement a shared modality-aware mechanism in Perturber for initializer gradient_modifier projector composer and optim_params.
|
Abandoned in favor of #115 |
2 participants
What does this PR do?
This PR adds a modality-aware composer
Composer.Type of change
Please check all relevant options.
Testing
Please describe the tests that you ran to verify your changes. Consider listing any relevant details of your test configuration.
pytest tests/test_composer.pyBefore submitting
pre-commit run -acommand without errorsDid you have fun?
Make sure you had fun coding 🙃