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

Adding colors to the IHC #2279

Merged
merged 2 commits into from Sep 5, 2016
Merged

Adding colors to the IHC #2279

merged 2 commits into from Sep 5, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 31, 2016

Description

Following google group discussion, I added the colors to the IHC gallery.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

@stefanv
Copy link
Member

stefanv commented Aug 31, 2016

Thanks, @cespenel! If anyone uses this in their work, please review this PR.

@codecov-io
Copy link

codecov-io commented Sep 1, 2016

Current coverage is 90.59% (diff: 100%)

Merging #2279 into master will not change coverage

@@             master      #2279   diff @@
==========================================
  Files           304        304          
  Lines         21425      21425          
  Methods           0          0          
  Messages          0          0          
  Branches       1844       1844          
==========================================
  Hits          19411      19411          
  Misses         1661       1661          
  Partials        353        353          

Powered by Codecov. Last update 1e94bac...9b11337

# Create an artificial color close to the orginal one
cmap_Hema = LinearSegmentedColormap.from_list('mycmap', ['white', 'navy'])
cmap_DAB = LinearSegmentedColormap.from_list('mycmap', ['white',
'saddlebrown'])
Copy link
Member

Choose a reason for hiding this comment

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

Please, follow PEP8 for the indentation here and below.

@soupault
Copy link
Member

soupault commented Sep 1, 2016

One remark from my side.
I'm not an expert, but this looks better and the code is ok. So, if anyone'd like to accept this, feel free to merge. 👍

@soupault soupault added 📄 type: Documentation Updates, fixes and additions to documentation ⏩ type: Enhancement Improve existing features labels Sep 1, 2016
@@ -35,13 +42,13 @@
ax[0].imshow(ihc_rgb)
ax[0].set_title("Original image")

ax[1].imshow(ihc_hed[:, :, 0], cmap=plt.cm.gray)
ax[1].imshow(ihc_hed[:, :, 0], cmap=cmap_Hema)
Copy link
Member

Choose a reason for hiding this comment

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

or cmap_hema?

@sciunto
Copy link
Member

sciunto commented Sep 1, 2016

One remark also mon my side but I'm don't use this feature.

@ghost
Copy link
Author

ghost commented Sep 1, 2016

Thank you, I cleaned it.

@emmanuelle
Copy link
Member

Hum, on the one hand this PR looks fine to me and I suggest to merge it; on the other hand the example seems very very domain-specific. I wonder if such functions (like data.immunohistochemistry, color.rgb2hed) should not be moved to separate projects within the scikit-image organisation (we could have skimage-bio, skimage-tomo -- imported as skimbio and skimtomo :-)).

What do you think?

About this specific example, it was written by @spotter quite a while ago. @cespenel, do you know if a lot of people could be using these functions, or is it very specific to immunohistochemistry?

@ghost
Copy link
Author

ghost commented Sep 1, 2016

@emmanuelle, I only used the module for histology and I'm not sure what other application it could have. I do agree that having a Scikit BioImage gallery (or something like that) for confocal images etc. would be very useful, at least in my domain :-) ...

@emmanuelle
Copy link
Member

@cespenel thanks for your insights.

@scikit-image/core what do you think about projects that would be more domain-specific?

@emmanuelle
Copy link
Member

@scikit-image/core this PR looks good and is an improvement so I'm merging it. We can have the discussion about domain-specific projects somewhere else.

@emmanuelle emmanuelle merged commit bc1b9b2 into scikit-image:master Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features 📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants