Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for baseTypeLowest/Max/Smallest/Epsilon #348

Open
cary-ilm opened this issue Sep 11, 2023 · 3 comments
Open

Add tests for baseTypeLowest/Max/Smallest/Epsilon #348

cary-ilm opened this issue Sep 11, 2023 · 3 comments
Labels
good first issue Good for newcomers

Comments

@cary-ilm
Copy link
Member

Various class templates declare methods that return lowest/max/smallest/epsilon on the template base type, e.g.

V2f::baseTypeLowest()
V2f::baseTypeMax()
V2f::baseTypeSmallest()
V2f::baseTypeEpsilon()

These methods are not covered in the test suite. Entries should be added to the python test src/python/PyImathTest/pyImathTest.in

@cary-ilm cary-ilm added the good first issue Good for newcomers label Sep 11, 2023
@ghost
Copy link

ghost commented Sep 12, 2023

Hi, If I have read the codebase correctly, pyImathTest.in contains the tests specific to the python bindings of the Imath library.

And the C++ class templates methods that, return the lowest/max/smallest/epsilon values of the base type, and where these methods are binded into python, eg.

.def("baseTypeEpsilon", &Matrix44<T>::baseTypeEpsilon,"baseTypeEpsilon() epsilon value of the base type of the vector")

must be tested in python similar to:

import imath

M44dEpsilon = imath.M44d.baseTypeEpsilon() 

DBL_EPS == M44dEpsilon

Is this correct?

@cary-ilm
Copy link
Member Author

The python-based tests have the dual advantage of validating both the binding code and the underlying Imath functionality, so the majority of the coverage of the test suite actually comes from pyImathTest.in. Your suggestion is along the lines of what I had in mind, replicated for each of the class instantiations.

I noted this issue from the SonarCloud coverage analysis report: https://sonarcloud.io/component_measures?metric=uncovered_lines&view=list&id=AcademySoftwareFoundation_Imath

A straightforward issue as a potential task for the ASWF Dev Days event.

@ghost
Copy link

ghost commented Sep 13, 2023

From what I see in the pyImathTest.in, when trying to integrate baseTypeLimits, there is a fundemental mismatch in the use of types.

We have in pyImathTest.in:

VecBaseType = {}
VecBaseType[V2s] = int
VecBaseType[V2i] = int
VecBaseType[V2f] = float
VecBaseType[V2d] = float
VecBaseType[V3s] = int
VecBaseType[V3i] = int
VecBaseType[V3f] = float
VecBaseType[V3d] = float

But, as you know, Python "float" is actually C/C++ double and we have int64_t and short in C/C++ types, which in the tests, are being treated as the intuitively closest type in Python.

However in the main codebase we have class methods similar to:

IMATH_HOSTDEVICE constexpr static T baseTypeLowest () IMATH_NOEXCEPT
    {
        return std::numeric_limits<T>::lowest ();
    }

Where T can be any one of C/C++ float, double, int, short, int64_t or even possibly unsigned char.

Without significant changes to possibly the entire pyImathTest.in and some minor additions to the imathmodule.cpp file, I don't think it is possible to correctly test the baseTypeLimits.

For instance, we need to add the below code into the imathmodule.cpp file to start:

scope().attr("SHT_MIN")      = std::numeric_limits<short>::min();
scope().attr("SHT_MAX")      = std::numeric_limits<short>::max();
scope().attr("SHT_LOWEST")   = std::numeric_limits<short>::lowest();
scope().attr("SHT_EPS")      = std::numeric_limits<short>::epsilon();

scope().attr("I64_MIN")    = std::numeric_limits<int64_t>::min();
scope().attr("I64_MAX")    = std::numeric_limits<int64_t>::max();
scope().attr("I64_LOWEST") = std::numeric_limits<int64_t>::lowest();
scope().attr("I64_EPS")    = std::numeric_limits<int64_t>::epsilon();

To give a concrete example why we would have to change the pyImathTest.in file, we have tests on various templated classes like so:

def testV2 ():

    print ("V2i")
    testV2x (V2i)
    print ("V2f")
    testV2x (V2f)
    print ("V2d")
    testV2x (V2d)

But the BaseTypes defined at te start of the file means that we will have errors in testing the limits of V2f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant