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

Color spaces C1C2C3 and MAXRGB added. #2915

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

imransalam
Copy link

Description

This is an addition to color spaces, I have added two color spaces, i-e C1C2C3 and MAXRGB.

figure_1-1

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

https://arxiv.org/abs/1702.05421
https://docs.gimp.org/en/plug-in-max-rgb.html

For reviewers

(Don't remove the checklist below.)

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@pep8speaks
Copy link

pep8speaks commented Dec 8, 2017

Hello @imransalam! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-12-06 11:44:21 UTC

@imransalam imransalam closed this Dec 8, 2017
@imransalam imransalam reopened this Dec 8, 2017
@imransalam imransalam closed this Dec 8, 2017
@imransalam imransalam reopened this Dec 8, 2017
@imransalam
Copy link
Author

@pep8speaks Hi, I've updated the skimage/color/colorconv.py with PEP8 standards in the latest commit.

@codecov-io
Copy link

codecov-io commented Dec 8, 2017

Codecov Report

Merging #2915 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2915      +/-   ##
==========================================
+ Coverage   86.04%   86.04%   +<.01%     
==========================================
  Files         337      337              
  Lines       27032    27046      +14     
==========================================
+ Hits        23259    23272      +13     
- Misses       3773     3774       +1
Impacted Files Coverage Δ
skimage/color/__init__.py 100% <ø> (ø) ⬆️
skimage/color/colorconv.py 99.11% <100%> (+0.03%) ⬆️
skimage/draw/_random_shapes.py 95.06% <0%> (-1.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2a6094...c19ddd2. Read the comment docs.

@soupault soupault modified the milestones: 0.15, 0.16 Apr 20, 2019
@hmaarrfk
Copy link
Member

hmaarrfk commented Jul 6, 2019

This looks really cool.

Sorry that this slipped the radar. Are you still interested in adding this? It would definitely need some tests that ensure:

  1. The mathematical correctness.
  2. The range of inputs (Grayscale vs RGB) we care for are adequately addressed.

@imransalam
Copy link
Author

@hmaarrfk Hey. Sure let me know what tests you want to perform.

@hmaarrfk
Copy link
Member

hmaarrfk commented Jul 6, 2019

Cool, take a look at
https://github.com/scikit-image/scikit-image/blob/master/skimage/color/tests/test_colorconv.py#L497

essentially, you will want to add your function, each prefixed with test.

Probably in that same directory.

Don't worry about putting them in a class that is some old stuff that was required by nose.

Generally, you might want to find an other way to compute the result. That could be:

  1. finding a less efficient way of computing it.
  2. Copy pasting the output of a manually checked result.

See what you can think of on the test side.

@@ -41,7 +41,7 @@
:author: Travis Oliphant (XYZ and RGB CIE functions)
:author: Matt Terry (lab2lch)
:author: Alex Izvorski (yuv2rgb, rgb2yuv and related)

Copy link
Member

Choose a reason for hiding this comment

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

I think generally we are now adding each person's contributions to the file CONTRIBUTORS.txt.

@stefanv should Imram add his name here too for this module?


References
----------
.. [1] https://arxiv.org/abs/1702.05421
Copy link
Member

Choose a reason for hiding this comment

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

This reference needs to be fleshed out some more. I wish we could convert bib files to our format easily.

--------
>>> from skimage import data
>>> img = data.astronaut()
>>> img_c1c2c3 = rgb2c1c2c3(img)
Copy link
Member

Choose a reason for hiding this comment

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

This example is probably better shown in a gallery.


References
----------
.. [1] https://docs.gimp.org/en/plug-in-max-rgb.html
Copy link
Member

Choose a reason for hiding this comment

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

This should at least be given a title and a retrieved on time stamp.


Examples
--------
>>> from skimage import data
Copy link
Member

Choose a reason for hiding this comment

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

See if you can adapt your code to fit in a new file in this directory.

https://github.com/scikit-image/scikit-image/tree/master/doc/examples/color_exposure

@hmaarrfk
Copy link
Member

hmaarrfk commented Jul 6, 2019

@imransalam before working on anything, I would see if anybody else from @scikit-image/core has objection to adding this kind of functionality. Color space has been quite contentious here as the API has been inconsistent and some have even proposed moving to an other library. I cant see to recall the exact conversation though.

@@ -679,6 +679,85 @@ def rgb2xyz(rgb):
return _convert(xyz_from_rgb, arr)



def rgb2c1c2c3(rgb):
"""RGB to C1C2C3 color space conversion.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would welcome a short decribtion of what the C1C2C3 is... giving reference is nice but keep it easy to use :)

>>> img_c1c2c3 = rgb2c1c2c3(img)
"""
out = np.arctan(rgb / np.dstack((
np.maximum(rgb[..., 1], rgb[..., 2]) + 0.000000001,
Copy link
Contributor

Choose a reason for hiding this comment

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



def rgb2maxrgb(rgb):
"""RGB to MAXRGB color space conversion.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would welcome a short description of what the MAXRGB is... giving reference is nice but keep it easy to use :)

R = rgb[:, :, 0]
G = rgb[:, :, 1]
B = rgb[:, :, 2]
M = np.maximum( np.maximum(R, G), B)
Copy link
Contributor

Choose a reason for hiding this comment

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

np.max(rgb, axis=-1) is easier to read/undestand

Updated the description for C1C2C3 and MaxRGB. Added np.finfo(float).eps instead of adding a small floating point myself. Used the max with axis=-1.
Added test cases for both maxrgb and c1c2c3. Manual testing.
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.

None yet

7 participants