Skip to content
This repository was archived by the owner on Apr 10, 2024. It is now read-only.

Conversation

BadrYoubiIdrissi
Copy link
Contributor

@BadrYoubiIdrissi BadrYoubiIdrissi commented Jun 26, 2019

Modified the image parametrization to accept any number of channels.

The following choices were made :

  • No decorrelation is done across channels.
  • A sigmoid is applied on all channels.

On top of the main channels modification, there is a commit to fix a bug switching width and height.

@review-notebook-app
Copy link

Check out this pull request on ReviewNB: https://app.reviewnb.com/tensorflow/lucid/pull/179

You'll be able to see visual diffs and write comments on notebook cells. Powered by ReviewNB.

@coveralls
Copy link

coveralls commented Jun 26, 2019

Pull Request Test Coverage Report for Build 534

  • 12 of 14 (85.71%) changed or added relevant lines in 2 files are covered.
  • 27 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 70.492%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lucid/optvis/param/images.py 11 13 84.62%
Files with Coverage Reduction New Missed Lines %
lucid/misc/channel_reducer.py 13 72.34%
lucid/optvis/objectives.py 14 84.08%
Totals Coverage Status
Change from base Build 524: 0.2%
Covered Lines: 1763
Relevant Lines: 2501

💛 - Coveralls

Copy link
Contributor

@colah colah left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for adding support for this. :)

else:
return constrain_L_inf(2*t-1)/2 + 0.5

def arbitrary_channels_to_rgb(*args, channels=None, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand when I'd use this function in practice. Is it just for the tests? Maybe add a comment. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly :) It's a funky parametrization to test more than 3 channels

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! Consider adding a brief comment, or even moving into the tests? Otherwise, LGTM!

@BadrYoubiIdrissi BadrYoubiIdrissi changed the title WIP : Any number of channels Any number of channels Jun 27, 2019
@BadrYoubiIdrissi
Copy link
Contributor Author

@colah It's done :) I have moved the arbitrary_channels_to_rgb function to the tests as you suggested alongside with an integration level test. I also added the grayscale_image_rgb to the param package init for direct access. Thank you for reviewing !

@colah
Copy link
Contributor

colah commented Jun 27, 2019

Nice! Thanks for the PR. :)

@colah colah merged commit 8df43ea into tensorflow:master Jun 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants