Python unit test module LookTest update

[meimchu]

Result of the LookTest module update:
test_constructor_with_keyword (LookTest.LookTest) ... ok test_constructor_without_keyword (LookTest.LookTest) ... ok test_constructor_without_parameter (LookTest.LookTest) ... ok test_constructor_wrong_parameter_type (LookTest.LookTest) ... ok test_description (LookTest.LookTest) ... Segmentation fault: 11

The wrong type tests in setName(), setProcessSpace(), and setDescription() will trigger a segmentation fault. That seems like a really harsh rejection and perhaps it would be nice to have it throw exceptions instead of a crash? The rest of the test are fine and should pass without issues.

[michdolan]

If it's crashing that's a bug. I'll take a look at the bindings.

[michdolan]

Found the cause of the segfault. For certain argument types, pybind11 will convert None to nullptr, which OCIO is not happy about. The fix is to explicitly tell pybind11 when an argument can't support this implicit None conversion, as documented here:
https://pybind11.readthedocs.io/en/stable/advanced/functions.html#allow-prohibiting-none-arguments

I've tested this in for Look and BuiltinTransform (which had the same issue in my tests), and it fixes it. Following the fix, these calls raise a TypeError instead. You can fix this for Look in this PR. Just make the following change to PyLook.cpp:

Change FROM:

        .def("getName", &Look::getName)
        .def("setName", &Look::setName, "name"_a)
        .def("getProcessSpace", &Look::getProcessSpace)
        .def("setProcessSpace", &Look::setProcessSpace, "processSpace"_a)
        .def("getTransform", &Look::getTransform)
        .def("setTransform", &Look::setTransform, "transform"_a)
        .def("getInverseTransform", &Look::getInverseTransform)
        .def("setInverseTransform", &Look::setInverseTransform, "transform"_a)
        .def("getDescription", &Look::getDescription)
        .def("setDescription", &Look::setDescription, "description"_a);

TO (note the 3 added .none(false)):

        .def("getName", &Look::getName)
        .def("setName", &Look::setName, "name"_a.none(false))
        .def("getProcessSpace", &Look::getProcessSpace)
        .def("setProcessSpace", &Look::setProcessSpace, "processSpace"_a.none(false))
        .def("getTransform", &Look::getTransform)
        .def("setTransform", &Look::setTransform, "transform"_a)
        .def("getInverseTransform", &Look::getInverseTransform)
        .def("setInverseTransform", &Look::setInverseTransform, "transform"_a)
        .def("getDescription", &Look::getDescription)
        .def("setDescription", &Look::setDescription, "description"_a.none(false));

[michdolan]

Looks good Mei! I made some very minor suggestions for clarity. Otherwsie LGTM.

[meimchu]

Just a quick follow up question. If I were to print(OCIO.Look()), this is the result I get:
<Look name=, processSpace=>
Should it be more like <Look name=, processSpace=, transform=None, inverseTransform=None, description=>

Would it more clear to list all parameters in its default order for ease of constructor readability?

[michdolan]

The print() result echos the C++ lib's ostream operator<< result,

[doug-walker]

It looks like Look.cpp operator<< is unchanged from v1. As is the case with many of the op<<, some members are not printed if they have empty/default values. Important members (name and process space in this case) are printed, however, even if empty. The names seem to be sorted based on importance rather than alphabetically. Doesn't the order already match the python constructor? What order do you think would be better?

I'm noticing that we added a description member to Look but we did not update the op<< to print it, so that is a minor bug to be fixed.

[meimchu]

Per discussion today in the meeting, I can understand that the string representation of Look() object is just showing the more important bits. Just to elaborate more on what I was referring to earlier:

>>> print(OCIO.Look())
<Look name=, processSpace=>

By default, it pretty much has nothing. So now I purposely pass in a wrong type parameter so that I know exactly what Look() object parameters there are. I was only wondering if there is a more elegant way to display what parameters are available in this object otherwise (besides using print dir(lk))?

>>> lk = OCIO.Look(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. PyOpenColorIO.Look()
    2. PyOpenColorIO.Look(name: str = '', processSpace: str = '', transform: PyOpenColorIO.Transform = None, inverseTransform: PyOpenColorIO.Transform = None, description: str = '')

>>> lk = OCIO.Look(description="Hello world")
>>> print(lk)
<Look name=, processSpace=>

I put in a description parameter and it doesn't get displayed.

But ultimately, I get the desire to only show the more important stuff per today's meeting discussion!

[BernardLefebvre]

it could be nice to sort these in alphabetical order

[BernardLefebvre]

alphabetical order?

[doug-walker]

@meimchu, I think you raise a good point that it would be nice for python users to easily see what are the available members to set. But at the same time, we want to keep the printed values compact and not print seldom used members that are at their default value.

Perhaps the best solution is for the doc string to list the available members and what they do?