Skip to content

Optim-wip: Miscellaneous Changes & Fixes #827

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

Merged
merged 12 commits into from
May 15, 2022

Conversation

ProGamerGov
Copy link
Contributor

@ProGamerGov ProGamerGov commented Dec 20, 2021

This PR is mostly just the miscellaneous changes from the old atlas PRs.

  • Handle depreciations in favor of the new linalg module. Changes made in ToRGB.klt_transform and nchannels_to_rgb.
  • Fix device issue with nchannels_to_rgb function. Previously it wouldn't work on the GPU. Also added new and improved tests.
  • Change direction vector variable name to vec in Direction, NeuronDirection, & TensorDirection. This should improve readability.
  • Speed up dataset cov color matrix calculations.
  • Support cov color matrix creation with fewer or more than 3 color channels. While this change isn't needed it Captum at the moment, it will hopefully help make it easier for researchers to deal with non RGB models.

In addition to the old PR changes, this PR also fixes the following:

  • Added documentation for get_model_layers, collect_activations, Conv2dSame, & get_neuron_pos as they were all missing documentation.

  • Fixed image_cov and the dataset tests.

  • Renamed utils/image/dataset.py to utils/image/test_dataset.py as the lack of a test_ prefix was causing the tests not to be run.

  • Renamed utils/image/common.py to utils/image/test_common.py as the lack of a test_ prefix was causing the tests not to be run.

  • Added missing _dot_cossim tests.

* `get_model_layers`, `collect_activations`, `Conv2dSame`, & `get_neuron_pos` were all missing documentation.
@ProGamerGov
Copy link
Contributor Author

So, one thing that I'm not sure about is whether there should be an empty line between 'Args:' and the arguments:

# Without empty line after 'Args:'
def some_function():
    """
    Some description.

    Args:
        x (type): Stuff about the variable.
        y (type): Stuff about the variable.

    Returns:
        x (type): Stuff about the return variable.
    """

vs

# With empty line after 'Args:'
def some_function():
    """
    Some description.

    Args:

        x (type): Stuff about the variable.
        y (type): Stuff about the variable.

    Returns:
        x (type): Stuff about the return variable.
    """

Captum seems to use both formats at the moment, though that could be fixed easily enough with regex.

@ProGamerGov ProGamerGov force-pushed the optim-wip-miscellaneous-pr branch 2 times, most recently from 7a3239a to 47d7a1b Compare January 10, 2022 15:45
* Fixed `image_cov` and the dataset tests.
* Renamed `utils/image/dataset.py` to `utils/image/test_dataset.py` as the lack of a `test_` prefix was causing the tests not to be run.
* Renamed `utils/image/common.py` to `utils/image/test_common.py` as the lack of a `test_` prefix was causing the tests not to be run.
* Added missing `_dot_cossim` tests.
* Moved the `hue_to_rgb` function outside of `nchannels_to_rgb` for JIT support.
* Fixed `nchannels_to_rgb` and `hue_to_rgb` functions.
* Fixed `Direction` loss objective assert.
@ProGamerGov
Copy link
Contributor Author

ProGamerGov commented Feb 1, 2022

@NarineK The optim-wip branch does have a set version of black like the master branch, and thus a new update is causing the lint_test_py36_pip test to fail at the linting stage. The new update for the black code formatter removed the spacing around power operators: psf/black#2726. This change affects the following files, two of which are not optim-wip specific files:

captum/attr/_core/noise_tunnel.py
captum/attr/_utils/stat.py
captum/optim/_param/image/images.py
captum/optim/_core/loss.py
tests/optim/core/test_loss.py

@NarineK NarineK merged commit ba84783 into pytorch:optim-wip May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants