Skip to content

Add return_index_map argument in ssim()#56771

Closed
CohenAriel wants to merge 1 commit into
tensorflow:masterfrom
CohenAriel:add-ssim-index-map-option
Closed

Add return_index_map argument in ssim()#56771
CohenAriel wants to merge 1 commit into
tensorflow:masterfrom
CohenAriel:add-ssim-index-map-option

Conversation

@CohenAriel
Copy link
Copy Markdown
Contributor

@CohenAriel CohenAriel commented Jul 14, 2022

Closes #53115

@google-ml-butler google-ml-butler Bot added the size:M CL Change Size: Medium label Jul 14, 2022
@google-ml-butler google-ml-butler Bot requested a review from rohan100jain July 14, 2022 13:48
@google-ml-butler google-ml-butler Bot added the awaiting review Pull request awaiting review label Jul 14, 2022
@gbaned gbaned added the comp:ops OPs related issues label Jul 14, 2022
@rohan100jain rohan100jain requested review from SeeForTwo and jsmeredith and removed request for rohan100jain July 27, 2022 20:44
@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 27, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 27, 2022
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Jul 29, 2022
@gbaned
Copy link
Copy Markdown
Contributor

gbaned commented Aug 2, 2022

Here are the internal errors, @CohenAriel can you please verify ? Thank you!

Traceback (most recent call last):
File "/tensorflow/python/ops/image_ops_test.py", line 5801, in testWithIndexMap
self.assertAllEqual(ssim_global, math_ops.reduce_mean(ssim_locals, axes))
File "/tensorflow/python/framework/test_util.py", line 1571, in decorated
return f(*args, **kwds)
File "/tensorflow/python/framework/test_util.py", line 3292, in assertAllEqual
np.testing.assert_array_equal(a, b, err_msg="\n".join(msgs))
File "/py/numpy/testing/_private/utils.py", line 934, in assert_array_equal
assert_array_compare(operator.eq, x, y, err_msg=err_msg,
File "/py/numpy/testing/_private/utils.py", line 844, in assert_array_compare
raise AssertionError(msg)
AssertionError:
Arrays are not equal

not equal where = (array([0]),)
not equal lhs = array([0.06525472], dtype=float32)
not equal rhs = array([0.06525473], dtype=float32)
Mismatched elements: 1 / 1 (100%)
Max absolute difference: 7.450581e-09
Max relative difference: 1.1417688e-07
x: array([0.065255], dtype=float32)
y: array([0.065255], dtype=float32)

@CohenAriel
Copy link
Copy Markdown
Contributor Author

CohenAriel commented Aug 2, 2022

Weird. Am I running the wrong command?

bazel test -k tensorflow/python:image_ops_test --test_filter=*SSIMTest.testWithIndexMap*

resulted in

INFO: Options provided by the client:
  Inherited 'common' options: --isatty=1 --terminal_columns=191
INFO: Reading rc options for 'test' from /home/ariel/Projects/tensorflow/.bazelrc:
  Inherited 'common' options: --experimental_repo_remote_exec
INFO: Reading rc options for 'test' from /home/ariel/Projects/tensorflow/.bazelrc:
  Inherited 'build' options: --define framework_shared_object=true --define=use_fast_cpp_protos=true --define=allow_oversize_protos=true --spawn_strategy=standalone -c opt --announce_rc --define=grpc_no_ares=true --noincompatible_remove_legacy_whole_archive --enable_platform_specific_config --define=with_xla_support=true --config=short_logs --config=v2 --define=no_aws_support=true --define=no_hdfs_support=true --experimental_cc_shared_library --experimental_link_static_libraries_once=false --deleted_packages=tensorflow/compiler/mlir/tfrt,tensorflow/compiler/mlir/tfrt/benchmarks,tensorflow/compiler/mlir/tfrt/jit/python_binding,tensorflow/compiler/mlir/tfrt/jit/transforms,tensorflow/compiler/mlir/tfrt/python_tests,tensorflow/compiler/mlir/tfrt/tests,tensorflow/compiler/mlir/tfrt/tests/ir,tensorflow/compiler/mlir/tfrt/tests/analysis,tensorflow/compiler/mlir/tfrt/tests/jit,tensorflow/compiler/mlir/tfrt/tests/lhlo_to_tfrt,tensorflow/compiler/mlir/tfrt/tests/lhlo_to_jitrt,tensorflow/compiler/mlir/tfrt/tests/tf_to_corert,tensorflow/compiler/mlir/tfrt/tests/tf_to_tfrt_data,tensorflow/compiler/mlir/tfrt/tests/saved_model,tensorflow/compiler/mlir/tfrt/transforms/lhlo_gpu_to_tfrt_gpu,tensorflow/core/runtime_fallback,tensorflow/core/runtime_fallback/conversion,tensorflow/core/runtime_fallback/kernel,tensorflow/core/runtime_fallback/opdefs,tensorflow/core/runtime_fallback/runtime,tensorflow/core/runtime_fallback/util,tensorflow/core/tfrt/common,tensorflow/core/tfrt/eager,tensorflow/core/tfrt/eager/backends/cpu,tensorflow/core/tfrt/eager/backends/gpu,tensorflow/core/tfrt/eager/core_runtime,tensorflow/core/tfrt/eager/cpp_tests/core_runtime,tensorflow/core/tfrt/gpu,tensorflow/core/tfrt/run_handler_thread_pool,tensorflow/core/tfrt/runtime,tensorflow/core/tfrt/saved_model,tensorflow/core/tfrt/graph_executor,tensorflow/core/tfrt/saved_model/tests,tensorflow/core/tfrt/tpu,tensorflow/core/tfrt/utils
INFO: Found applicable config definition build:short_logs in file /home/ariel/Projects/tensorflow/.bazelrc: --output_filter=DONT_MATCH_ANYTHING
INFO: Found applicable config definition build:v2 in file /home/ariel/Projects/tensorflow/.bazelrc: --define=tf_api_version=2 --action_env=TF2_BEHAVIOR=1
INFO: Found applicable config definition build:linux in file /home/ariel/Projects/tensorflow/.bazelrc: --copt=-w --host_copt=-w --define=PREFIX=/usr --define=LIBDIR=$(PREFIX)/lib --define=INCLUDEDIR=$(PREFIX)/include --define=PROTOBUF_INCLUDE_PATH=$(PREFIX)/include --cxxopt=-std=c++17 --host_cxxopt=-std=c++17 --config=dynamic_kernels --distinct_host_configuration=false --experimental_guard_against_concurrent_changes
INFO: Found applicable config definition build:dynamic_kernels in file /home/ariel/Projects/tensorflow/.bazelrc: --define=dynamic_loaded_kernels=true --copt=-DAUTOLOAD_DYNAMIC_KERNELS
INFO: Analyzed 2 targets (0 packages loaded, 0 targets configured).
INFO: Found 2 test targets...
INFO: Elapsed time: 8.992s, Critical Path: 1.66s
INFO: 33 processes: 1 internal, 32 local.
INFO: Build completed successfully, 33 total actions
//tensorflow/python:image_ops_test_cpu                                   PASSED in 1.7s
  Stats over 16 runs: max = 1.7s, min = 1.2s, avg = 1.5s, dev = 0.1s
//tensorflow/python:image_ops_test_gpu                                   PASSED in 1.7s
  Stats over 16 runs: max = 1.7s, min = 1.3s, avg = 1.5s, dev = 0.1s

Executed 2 out of 2 tests: 2 tests pass.
INFO: Build completed successfully, 33 total actions

@google-ml-butler google-ml-butler Bot removed the ready to pull PR ready for merge process label Aug 2, 2022
@CohenAriel CohenAriel force-pushed the add-ssim-index-map-option branch from e43cef1 to bd932e3 Compare August 2, 2022 20:55
@gbaned
Copy link
Copy Markdown
Contributor

gbaned commented Aug 3, 2022

Hi @SeeForTwo Can you please assist on above comments from @CohenAriel. Thank you!

@SeeForTwo
Copy link
Copy Markdown
Contributor

SeeForTwo commented Aug 3, 2022

When @gbaned ran the test, there appeared to be a trivial floating point rounding difference:

not equal lhs = array([0.06525472], dtype=float32)
not equal rhs = array([0.06525473], dtype=float32)
sMax absolute difference: 7.450581e-09
Max relative difference: 1.1417688e-07

If you agree that this is just a rounding issue (how rounding occurs on different computers), I would suggest changing the test to use self.assertAllClose(ssim_global, math_ops.reduce_mean(ssim_locals, axes)) instead of self.assertAllEqual(ssim_global, math_ops.reduce_mean(ssim_locals, axes)). The default tolerances for "close" are 1e-6. (In general, I favor using assertAllClose over assertAllEqual when comparing floats.)

@CohenAriel
Copy link
Copy Markdown
Contributor Author

CohenAriel commented Aug 3, 2022

Makes sense. I'll try that.

@CohenAriel CohenAriel force-pushed the add-ssim-index-map-option branch from bd932e3 to 3385069 Compare August 3, 2022 18:02
@gbaned gbaned requested a review from SeeForTwo August 4, 2022 11:14
@google-ml-butler google-ml-butler Bot added the awaiting review Pull request awaiting review label Aug 4, 2022
@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 4, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 4, 2022
@rohan100jain rohan100jain self-requested a review August 8, 2022 23:31
@rohan100jain
Copy link
Copy Markdown
Member

rohan100jain commented Aug 8, 2022

@tensorflow/api-owners

@SeeForTwo
Copy link
Copy Markdown
Contributor

SeeForTwo commented Aug 8, 2022 •
edited
Loading

@CohenAriel, because adding this optional argument changes the public API, "the goldens" need to be updated.

Can you please try running:

bazel run tensorflow/tools/api/tests:api_compatibility_test  -- --update_goldens True

See "Py+XPP Test Suite - Ubuntu CPU, Python 3.9.." Details --> (ResultStore)--> tensorflow/rel/docker/github_presubmit/pycpp_cpu_py39 and search for "If this test fails, it means a change has been made to the public API" in the log.

@SeeForTwo
Copy link
Copy Markdown
Contributor

SeeForTwo commented Aug 8, 2022

After running that command, commit the changes that it (hopefully) creates.

@CohenAriel CohenAriel force-pushed the add-ssim-index-map-option branch from 3385069 to 2c9451c Compare August 9, 2022 10:23
@google-ml-butler google-ml-butler Bot removed the ready to pull PR ready for merge process label Aug 9, 2022
@CohenAriel CohenAriel force-pushed the add-ssim-index-map-option branch from 2c9451c to 8f5a1b1 Compare August 9, 2022 10:25
@CohenAriel CohenAriel requested a review from SeeForTwo August 9, 2022 10:29
@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 9, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 9, 2022
@google-ml-butler google-ml-butler Bot added the kokoro:force-run Tests on submitted change label Aug 11, 2022
@rohan100jain rohan100jain removed the request for review from jsmeredith August 11, 2022 16:30
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 11, 2022
@SeeForTwo
Copy link
Copy Markdown
Contributor

SeeForTwo commented Aug 11, 2022

For feedback/copybara — Google internal checks FAILED (i.e. since this is a public API change) could you please add a short summary of this change to RELEASE.md. My suggestion is in # Bug Fixes and Other Changes to add something like the following:

*   `tf.image`
    *   Added an optional parameter `return_index_map` to `tf.image.ssim` which
        causes the returned value to be the local SSIM map instead of the
        global mean.

Thank you for your patience!

@CohenAriel CohenAriel force-pushed the add-ssim-index-map-option branch from 8f5a1b1 to a2ed401 Compare August 11, 2022 22:30
@google-ml-butler google-ml-butler Bot removed the ready to pull PR ready for merge process label Aug 11, 2022
@CohenAriel
Copy link
Copy Markdown
Contributor Author

CohenAriel commented Aug 11, 2022

No problem. I hope it's fine now.

@google-ml-butler google-ml-butler Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 11, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 11, 2022
@google-ml-butler google-ml-butler Bot removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Aug 13, 2022
@CohenAriel CohenAriel deleted the add-ssim-index-map-option branch August 13, 2022 17:50
copybara-service Bot pushed a commit that referenced this pull request Aug 14, 2022
@tensorflower-gardener
Merge pull request #56771 from CohenAriel:add-ssim-index-map-option
edad112 
PiperOrigin-RevId: 467465476
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohan100jain rohan100jain rohan100jain approved these changes

+1 more reviewer

@SeeForTwo SeeForTwo SeeForTwo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gbaned gbaned

Labels

comp:ops OPs related issues size:M CL Change Size: Medium

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SSIM index maps

7 participants

@CohenAriel @gbaned @SeeForTwo @rohan100jain @mihaimaruseac @kokoro-team @tensorflowbutler