Implementation of Matrix22

[oxt3479]

I have written all possible functionality for the case of a 2x2 Matrix. Prior it was only 3x3 and 4x4. Functions that are not transferable to this case were omitted. These are limits of the Matrix22 format and include: Shear, Minors, Gaus-Jordan Inversion, and Translation.

Signed-off-by Owen Thompson (oxt3479)

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ owdt (53c5db0)

[lgritz]

Nice!

I see declarations for operator<<, but no implementation. I have not checked to see if any other methods are missing implementations.

Also, we'll want whatever unit tests exists for 3x3 to also have analogous ones for 2x2.

[lgritz]

I should have led with: Welcome! Thanks for the PR and this is the start of a really useful addition to the library.

[meshula]

Thanks for a clean implementation! Everything looks correct to me. I left a couple of comments for discussion.

When you get to the corresponding tests, I suggest adding tests for narrowing to make sure that for example assigning a double2x2 to a float2x2 doesn't emit compiler warnings. A point of discussion is whether narrowings warrant warnings, and if so, we would probably need to introduce api to make it possible to do narrowing via an explicit extra step such as adding a narrow_cast to our APIs. Narrowing could warrant a separate issue...

[lgritz]

Closes #387

[lgritz]

Can you comment on how much of this, if any, was copied from the OSL implementation (as suggested in #263)?

Nothing wrong with either copying it (though it should be attributed) or redoing it from scratch (I hope you didn't waste much time doing that). But it would be good to understand what ways this is the same or differs from that implementation.

[oxt3479]

The only thing I copied from the OSL implementation was the matrix inversion function. Everything else is a modified version of the corresponding Matrix33 functions with minimal adjustments made.

[oxt3479]

So I have pushed fixes for the issues I could at this time. As for the narrowing, I am addressing this in a project proposal for the Google Summer of Code for ASWF. I am not experienced with Unit testing so getting that all to play nice is going to take some time.

On another note: I wasn't certain if this pull needs to be accepted in order to have the proposal application considered? The deadline is tomorrow.

[lgritz]

As far as the requirement for the application, we only need the PR submitted. It doesn't need to fully complete the review and acceptance cycle before the deadline. So you are fine.

[cary-ilm]

Thank you for this! The unit tests are quite simple and straightforward. Take a look at https://github.com/AcademySoftwareFoundation/openexr/blob/master/IlmBase/ImathTest/testMatrix.cpp. This has a single "testMatrix()" function which constructs a series of matrices with specific values and performs various operations on them, followed by 'assert()' statements that validate the results are what you expect. As the comment at the top of the file indicates, most of the tests are actually implemented in python in PyIlmBase/PyImathTest, but the principle is the same: construct objects with known values, perform operations, call assert() to confirm that the operations performed as expected. Writing tests can be challenging because they can seem like pointless exercises in stating the obvious, but their value really appears later, when someone years from now makes a change that has unintended consequences, or when the code is compiled on some kind of different system with slightly different behavior. A little time spent now to ensure that every line of code is validated in a test will pay off at some point in the future.

…

On Mon, Mar 30, 2020 at 2:13 PM Larry Gritz ***@***.***> wrote: As far as the requirement for the application, we only need the PR submitted. It doesn't need to fully complete the review and acceptance cycle before the deadline. So you are fine. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#690 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFC3DGNYVRMZWWQ2DBC2ZCTRKEDO5ANCNFSM4LVWBASA> .

-- Cary Phillips | R&D Supervisor | ILM | San Francisco

[meshula]

The code is looking good. As a starting point on tests, a good first step is to simply run the existing tests. To do that, you first need to tell cmake to build them. That's done with the BUILD_TESTING flag. That won't run the tests, but the test programs will appear in reasonable places according to which platform you are on. At that point you can run them from the command line, individually, and the results should be self-descriptive. Maybe you could give that a go, just to run the Imath tests, and we can proceed from there?

[meshula]

this all looks great, gets a thumbs up from me :) Anyone else?

[lgritz]

LGTM by eye, but there are still a couple of CI tests that are failing.

[oxt3479]

Traceback (most recent call last):
File "/Users/runner/runners/2.165.2/work/1/s/PyIlmBase/PyImathTest/pyImathTest.in", line 10, in
import iex
ModuleNotFoundError: No module named 'iex'

I see this stack trace occurring when testing AppleClang. Will this potentially cause any issues when building testing on MacOS?

[meshula]

@xlietz Do we have automated testing of the Python bindings on macOS? The error @oxt3479 reports suggests that the PYTHONPATH variable hasn't been configured. Or, that the tests are running in a virtualenv, in which case the PYTHONPATH is ignored as some sort of security theatrics, and has to be set by trampoline script within the virtualenv that does a sys.path.append("/path/to/our/stuff") in order to work around the sandboxing. If this is the case, we could set an environment variable external named say, EXR_PYTHON_PACKAGE_PATH, to the script, and modify the test scripts to include, before any OpenEXR modules are included -

import os, sys
sys.path.append(os.getenv("EXR_PYTHON_PACKAGE_PATH"))

[xlietz]

@meshula apologies for the late reply. These tests were previously working, nothing in CI has changed nor can I see a particular merge that caused them to fail. They are failing both in the repo build as well as in all PRs so it is not related to this PR. Something must have changed in the MacOS image. I will investigate this weekend and will try your suggestion of setting an env var to fix the path.

[cary-ilm]

The Azure failure seems unrelated to the changes, so I'm merging it in now. Thanks!