Add turbo color map #1055

kalpitthakkar-lm · 2020-05-26T13:13:14Z

What this patch does to fix the issue.

This PR contains replacement of the current Jet colormap of matplotlib by adding the Google proposed Turbo Colormap as highlighted in #485 by @tk26eng in blueoil/common.py.

This colormap is for replacing the Jet colormap in matplotlib that is used for coloring heatmaps. So I added a new function that takes the heatmap as input and then returns the colored heatmap. As the function uses numpy, it should be called using tf.py_func interface.

The table for reference colors is a (256x3) matrix of sRGB colors and I saved it to a separate file blueoil/turbo_color_map.py.

Link to any relevant issues or pull requests.

Relevant issue: #485

bo-code-review-bot · 2020-05-26T13:13:18Z

This PR needs Approvals as follows.

Ownership Approval for / from iizukak, tkng, ruimashita
Readability Approval for Python from tkng, tsawada, tfujiwar

Please choose reviewers and requet reviews!

Click to see how to approve each reviews

You can approve this PR by triggered comments as follows.

Approve all reviews requested to you (readability and ownership) and LGTM review
Approval, LGTM
Approve all ownership reviews
Ownership Approval or OA
Approve all readability reviews
Readability Approval or RA
Approve specified review targets
- Example of Ownership Reviewer of /: Ownership Approval for / or OA for /
- Example of Readability Reviewer of Python: Readability Approval for Python or RA for Python
Approve LGTM review
LGTM

See all trigger comments

Please replace [Target] to review target

Ownership Approval
- Ownership Approval for [Target]
- OA for [Target]
- Ownership Approval
- OA
- Approval
Readability Approval
- Readability Approval for [Target]
- RA for [Target]
- [Target] Readability Approval
- [Target] RA
- Readability Approval
- RA
- Approval
LGTM
- LGTM
- lgtm

iizukak

@kalpitthakkar-lm Thanks. turbo colormap looks nice.

iizukak · 2020-05-27T13:03:53Z

blueoil/common.py

+# For replacing the Matplotlib Jet colormap, we use the Turbo color map
+# https://ai.googleblog.com/2019/08/turbo-improved-rainbow-colormap-for.html
+# The colormap allows for a large number of quantization levels:
+# https://github.com/blue-oil/blueoil/tree/master/docs/_static/turbo_cmap.png


Missing link. Do you have plan to add image to the documentation?

@iizukak Yes, I will add that image and upload to the commit.

iizukak · 2020-05-27T13:04:22Z

blueoil/turbo_color_map.py

@@ -0,0 +1,258 @@
+TURBO_CMAP_DATA = [


Maybe we need to set symlink to here
https://github.com/blue-oil/blueoil/tree/master/output_template/python/blueoil

Ah, I get it. That will make sure the tests will pass (the ones that have failed, because the symlinks are missing there).

Thank you!!

iizukak

@kalpitthakkar-lm
Sorry, Can you add license header to the new file?

like

blueoil/blueoil/common.py

Lines 1 to 15 in 5575a8a

    
           # -*- coding: utf-8 -*- 
        
           # Copyright 2018 The Blueoil Authors. All Rights Reserved. 
        
           # 
        
           # Licensed under the Apache License, Version 2.0 (the "License"); 
        
           # you may not use this file except in compliance with the License. 
        
           # You may obtain a copy of the License at 
        
           # 
        
           #     http://www.apache.org/licenses/LICENSE-2.0 
        
           # 
        
           # Unless required by applicable law or agreed to in writing, software 
        
           # distributed under the License is distributed on an "AS IS" BASIS, 
        
           # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. 
        
           # See the License for the specific language governing permissions and 
        
           # limitations under the License. 
        
           # =============================================================================

iizukak

OA

kalpitthakkar-lm · 2020-06-04T06:49:49Z

@iizukak The tests failed after I updated the branch by pulling from master. What should we do?
Are there other reviews required for merge?

iizukak · 2020-06-04T06:51:59Z

@kalpitthakkar-lm
CI sometimes unstable. Please wait for this trial.
If this trial also failed, We should contact to the infra team.

kalpitthakkar-lm · 2020-06-08T08:44:01Z

@tk26eng @iizukak Can someone now push this to master?

iizukak · 2020-06-09T00:11:50Z

@kalpitthakkar-lm This PR should get Python Readability Approval

a-hanamoto · 2020-06-09T03:35:27Z

blueoil/common.py

+# The colormap allows for a large number of quantization levels:
+# https://github.com/blue-oil/blueoil/tree/master/docs/_static/turbo_cmap.png
+# Implementation inspired from the following gist:
+# https://gist.github.com/mikhailov-work/ee72ba4191942acecc03fe6da94fc73f


Original codes are Apache 2.0 licensed.
To use Apache 2.0 licensed codes, you need to do the followings:
(Please see http://www.apache.org/licenses/LICENSE-2.0 4. Redistribution for further information)

You need to put a copy of the License somewhere

You need to clarify what you changed from the original

You need to leave the original copyright

You need to leave NOTICE files if any
I think there are some rules (e.g. where you should put the License files) in Blueoil, so please ask BO team that.

@iizukak Where should I put the license file / clarify what I have changed from the original?

a-hanamoto · 2020-06-09T03:38:25Z

blueoil/turbo_color_map.py

+# limitations under the License.
+# =============================================================================
+
+TURBO_CMAP_DATA = [


Same as above.

a-hanamoto · 2020-06-09T03:41:22Z

blueoil/common.py

+def interpolate(colormap, x):
+    x = max(0.0, min(1.0, x))
+    a = int(x*255.0)
+    b = min(255, a + 1)
+    f = x*255.0 - a
+    return [colormap[a][0] + (colormap[b][0] - colormap[a][0]) * f,
+            colormap[a][1] + (colormap[b][1] - colormap[a][1]) * f,
+            colormap[a][2] + (colormap[b][2] - colormap[a][2]) * f]
+
+
+def interpolate_or_clip(colormap, x):
+    if x < 0.0:
+        return [0.0, 0.0, 0.0]
+    elif x > 1.0:
+        return [1.0, 1.0, 1.0]
+    else:
+        return interpolate(colormap, x)


Will you need these methods?
I think only color_map_apply is sufficient.

Removed them.

a-hanamoto · 2020-06-09T03:45:44Z

blueoil/common.py

+    image_colored[image < 0.0] = 0.0
+    image_colored[image > 1.0] = 1.0


I think this won't happen since we clipped values before.

Correct, I fixed it.

a-hanamoto · 2020-06-09T03:47:28Z

blueoil/common.py

+    # If image range is [0, 1], continue ahead. Else convert to that range.
+    if image.max() > 1.0:
+        image = (image - image.min()) / image.max()


There is inconsistency between the comment and its code.
(The code does not check the min value)

We don't need to change the range of values.
I've updated the code.

a-hanamoto · 2020-06-09T03:48:03Z

blueoil/common.py

+    if image.max() > 1.0:
+        image = (image - image.min()) / image.max()
+
+    x = image.clip(0.0, 1.0)


If the comment is true, then you don't need this code.

a-hanamoto · 2020-06-09T03:50:34Z

blueoil/common.py

+        return interpolate(colormap, x)
+
+
+def color_map_apply(image):


I think apply_color_map is more straightforward.

Made the change.

a-hanamoto · 2020-06-09T04:04:02Z

blueoil/common.py

+    x = image.clip(0.0, 1.0)
+    a = (x * 255).astype(int)
+    b = (a + 1).clip(max=255)
+    f = x * 255.0 - a


numpy.modf will be helpful
https://numpy.org/doc/stable/reference/generated/numpy.modf.html

Wouldn't it be easier to understand this?

Although it is not a major issue, it seems redundant to calculate x * 255.0 twice.

Correct, let me update it to np.modf (I will add a comment on how it works above it though..)

…color_map

a-hanamoto

Commented

a-hanamoto

Please let me review after fixing license issues. Thank you!

iizukak · 2020-07-01T00:04:55Z

Sorry, I'm now checking the status

a-hanamoto

Please put a copy of the License somewhere
Please clarify what you changed from the original

kalpitthakkar-lm · 2020-07-03T06:14:51Z

Please put a copy of the License somewhere

They have not provided their LICENSE copy, but only the License Identifier (SPDX line) and a copyright line. I have included both in the colormap table file. Apache-2.0 LICENSE terms are available everywhere.

Please clarify what you changed from the original

I did not change anything in the colormap table. That's the only thing I have taken from the gist. The function apply_color_map is written by myself.

a-hanamoto

They have not provided their LICENSE copy, but only the License Identifier (SPDX line) and a copyright line. I have included both in the colormap table file. Apache-2.0 LICENSE terms are available everywhere.

Got it.
@iizukak Could you confirm whether it is OK or not?

I did not change anything in the colormap table. That's the only thing I have taken from the gist. The function apply_color_map is written by myself.

It seems you changed its format and name.

a-hanamoto · 2020-07-03T11:04:04Z

blueoil/turbo_color_map.py

@@ -0,0 +1,279 @@
+# -*- coding: utf-8 -*-
+# Copyright 2018 The Blueoil Authors. All Rights Reserved.


Suggested change

# Copyright 2018 The Blueoil Authors. All Rights Reserved.

# Copyright 2020 The Blueoil Authors. All Rights Reserved.

iizukak

Sorry for taking long time!
Ownership Approval

a-hanamoto

RA (reverse-shadowing)

iizukak · 2020-07-09T05:13:12Z

/ready

bo-mergebot · 2020-07-09T05:13:14Z

⏳Merge job is queued...

bo-mergebot · 2020-07-09T05:13:15Z

😥This PR is not approved yet.

kalpitthakkar-lm · 2020-07-10T02:49:14Z

@tsawada Can you have a look?

tsawada

Readability wise, this PR is okay.

Is this going to allow us to drop matplotlib dependency? If not, simply switching to inferno colormap would be easier.

tsawada · 2020-07-10T03:03:43Z

blueoil/turbo_color_map.py

+# Colormap table taken from the following gist:
+# https://gist.github.com/mikhailov-work/ee72ba4191942acecc03fe6da94fc73f
+# Copyright 2019 Google LLC.
+# SPDX-License-Identifier: Apache-2.0


so what's the license of this file? I feel it's weird having two license claims on this file.

Okay, I will remove the Blueoil LICENSE header, as the table in the file is from the gist so I'll put these four lines at the top.

tsawada · 2020-07-10T03:05:06Z

blueoil/common.py

+    #   >>> print(i.shape, f.shape)
+    #   >>> (6,) (6,)
+    #   >>> print(i)
+    #   >>> [ 1.  2.  4. 20.  6.  8.]
+    #   >>> print(f)
+    #   >>> [0.2  0.3  0.5  0.45 0.75 0.88]


some lines wouldn't have >>> on the interpreter.

Got it. Will update that.

kalpitthakkar-lm · 2020-07-11T03:24:03Z

@tsawada Yes this will allow us to drop matplotlib dependency (it's used for Jet colormap used in heatmaps).

tsawada

Readability Approval

kalpitthakkar-lm · 2020-07-13T11:15:29Z

@iizukak Please give the command to merge this PR once the status checks are completed. Thank you!

iizukak · 2020-07-14T04:45:58Z

/ready

bo-mergebot · 2020-07-14T04:45:59Z

⏳Merge job is queued...

Add turbo color map

d66314d

blueoil-butler bot added the CI: auto-run Run CI automatically label May 26, 2020

kalpitthakkar-lm added 2 commits May 27, 2020 01:10

Make changes for PR

551d906

Fix pep8 test erors

78763f9

iizukak reviewed May 27, 2020

View reviewed changes

kalpitthakkar-lm added 2 commits May 28, 2020 20:22

Add symlink and turbo_cmap in static docs

e5f0753

Update turbo_color_map symlink

5575a8a

kalpitthakkar-lm requested a review from iizukak May 29, 2020 07:05

iizukak reviewed Jun 1, 2020

View reviewed changes

kalpitthakkar-lm added 2 commits June 1, 2020 11:07

Add LICENSE header in turbo_color_map

7409446

Trailing whitespaces - copy paste problem

c6a15d8

kalpitthakkar-lm requested a review from iizukak June 1, 2020 03:24

iizukak approved these changes Jun 2, 2020

View reviewed changes

Merge branch 'master' into color_map

af60f92

Merge branch 'master' into color_map

53b92f5

Merge branch 'master' into color_map

e3c5d24

iizukak requested a review from a-hanamoto June 9, 2020 00:12

a-hanamoto suggested changes Jun 9, 2020

View reviewed changes

kalpitthakkar-lm added 2 commits June 9, 2020 20:54

Update according to comments. Remove unwanted code

7adde5a

Merge branch 'color_map' of github.com:kalpitthakkar-lm/blueoil into …

c6f29ff

…color_map

kalpitthakkar-lm requested a review from a-hanamoto June 10, 2020 05:05

a-hanamoto reviewed Jun 10, 2020

View reviewed changes

Use numpy modf

cae7235

kalpitthakkar-lm requested a review from a-hanamoto June 10, 2020 06:51

a-hanamoto reviewed Jun 10, 2020

View reviewed changes

Add the LICENSE identifier

a0ab393

kalpitthakkar-lm requested a review from a-hanamoto July 3, 2020 04:32

a-hanamoto suggested changes Jul 3, 2020

View reviewed changes

Put the LICENSE and copyright in colormap table file

70ca9b4

kalpitthakkar-lm requested a review from a-hanamoto July 3, 2020 09:16

a-hanamoto reviewed Jul 3, 2020

View reviewed changes

a-hanamoto requested a review from iizukak July 3, 2020 11:07

iizukak approved these changes Jul 9, 2020

View reviewed changes

Copyright year. Add changes

0b733f1

kalpitthakkar-lm requested a review from a-hanamoto July 9, 2020 04:14

a-hanamoto approved these changes Jul 9, 2020

View reviewed changes

a-hanamoto requested a review from tsawada July 9, 2020 04:16

Remove image we do not own copyright to

9f81c44

tsawada reviewed Jul 10, 2020

View reviewed changes

Example string: fix format

8fdbcbb

kalpitthakkar-lm requested a review from tsawada July 13, 2020 04:08

tsawada approved these changes Jul 13, 2020

View reviewed changes

Merge branch 'master' into color_map

cc2eb9b

bo-mergebot bot merged commit 08bf6c1 into blue-oil:master Jul 14, 2020

kalpitthakkar-lm deleted the color_map branch July 14, 2020 04:47

	# -- coding: utf-8 --
	# Copyright 2018 The Blueoil Authors. All Rights Reserved.
	#
	# Licensed under the Apache License, Version 2.0 (the "License");
	# you may not use this file except in compliance with the License.
	# You may obtain a copy of the License at
	#
	# http://www.apache.org/licenses/LICENSE-2.0
	#
	# Unless required by applicable law or agreed to in writing, software
	# distributed under the License is distributed on an "AS IS" BASIS,
	# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
	# See the License for the specific language governing permissions and
	# limitations under the License.
	# =============================================================================

		image_colored[image < 0.0] = 0.0
		image_colored[image > 1.0] = 1.0

		@@ -0,0 +1,279 @@
		# -- coding: utf-8 --
		# Copyright 2018 The Blueoil Authors. All Rights Reserved.

	# Copyright 2018 The Blueoil Authors. All Rights Reserved.
	# Copyright 2020 The Blueoil Authors. All Rights Reserved.

Add turbo color map #1055

Add turbo color map #1055

Conversation

kalpitthakkar-lm commented May 26, 2020 • edited Loading

What this patch does to fix the issue.

Link to any relevant issues or pull requests.

bo-code-review-bot bot commented May 26, 2020

iizukak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iizukak left a comment

Choose a reason for hiding this comment

iizukak left a comment

Choose a reason for hiding this comment

kalpitthakkar-lm commented Jun 4, 2020

iizukak commented Jun 4, 2020

kalpitthakkar-lm commented Jun 8, 2020

iizukak commented Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hanamoto left a comment

Choose a reason for hiding this comment

a-hanamoto left a comment

Choose a reason for hiding this comment

iizukak commented Jul 1, 2020

a-hanamoto left a comment

Choose a reason for hiding this comment

kalpitthakkar-lm commented Jul 3, 2020

a-hanamoto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iizukak left a comment

Choose a reason for hiding this comment

a-hanamoto left a comment

Choose a reason for hiding this comment

iizukak commented Jul 9, 2020

bo-mergebot bot commented Jul 9, 2020

bo-mergebot bot commented Jul 9, 2020

kalpitthakkar-lm commented Jul 10, 2020

tsawada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalpitthakkar-lm commented Jul 11, 2020

tsawada left a comment

Choose a reason for hiding this comment

kalpitthakkar-lm commented Jul 13, 2020

iizukak commented Jul 14, 2020

bo-mergebot bot commented Jul 14, 2020

kalpitthakkar-lm commented May 26, 2020 •

edited

Loading

iizukak commented Jun 9, 2020 •

edited

Loading

a-hanamoto left a comment •

edited

Loading