NVlabs / flip

A tool for visualizing and communicating the errors in rendered images.
498 stars 41 forks source link

Python Binding: inputParameters dict is shared between multiple calls to evaluate #32

Closed Latios96 closed 3 months ago

Latios96 commented 3 months ago

Hi,

I noticed that if no inputParameters dict is supplied to flip.evalute via the Python bindings, the default argument is used. Since the default argument only exists one time in memory and is mutated, this leads to unexpected results for subsequent calls.

Steps to reproduce:

  1. Download the attached images and Python Script flip-reproduce.zip
  2. Execute python reproduce.py. The script calls flip.evaluate from Python with no custom input parameters. For the first call, Flip will calculate start/stop exposure (in this case nan) and will store them in the inputParameters before returning them. The second flip does not supply its own inputParameters, but since the default argument was mutated in the previous call, the values from the previous call are used. This leads to different results:

Two flip calls:

python reproduce.py
0.0 {'ppd': 67.02064514160156, 'startExposure': nan, 'stopExposure': nan, 'numExposures': 2, 'tonemapper': 'aces'}
0.0 {'ppd': 67.02064514160156, 'startExposure': nan, 'stopExposure': nan, 'numExposures': 2, 'tonemapper': 'aces'}

Skipping the first flip call:

python reproduce.py 1
0.07767405360937119 {'ppd': 67.02064514160156, 'startExposure': -4.310622692108154, 'stopExposure': 8.824731826782227, 'numExposures': 14, 'tonemapper': 'aces'}

Expected behavior When not providing own inputParamaters, all calls to flip.evaluate should use defaults. Flip should not reuse the computed start/stop exposure and other parameters.

Workaround There is a workaround by passing an empty parameters dict to flip.evaluate in Python (see also workaround.py). This works as expected:

python reproduce_with_workaround.py
0.0 {'ppd': 67.02064514160156, 'startExposure': nan, 'stopExposure': nan, 'numExposures': 2, 'tonemapper': 'aces'}
0.07767405360937119 {'ppd': 67.02064514160156, 'startExposure': -4.310622692108154, 'stopExposure': 8.824731826782227, 'numExposures': 14, 'tonemapper': 'aces'}

I filed a PR with a proposed fix: #33