Testflo update

[marcomangano]

Purpose

As per title, this PR updates the testing infrastructure to use testflo.
I used a simplified parameterized approach from ADflow to run single core and parallel tests using the same runfile (but different .ref files!)
I also did my best to comment the code, but I am not 100% confident about the tests themselves, it looks they are stretching a few different kind of meshes.

Closes #33

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

I cross checked the values with the .ref files in the old format, they match

Checklist

• I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[ewu63]

A few general comments

• I personally would prefer a single reference file rather than one for serial and one for parallel, since some values are the same. This can also make it more explicit if the serial/parallel values do not mach when they are expected to match. However, I'm not sure exactly how to implement this for training, since you would have incomplete information when you train with just serial tests for example. Maybe we should shelf this and make it an enhancement in the BaseRegTest class. What do others think about this?
• Regarding the AttributeError during training, is this the same problem that @joanibal ran into for ADflow? What exactly is causing this problem? I wonder if there is a way to handle this without using the hacky solution
• How do you plan to add the complex tests? will it be a separate file, or will you use the conditional import approach? i.e. import either idwarp or idwarp_cs based on the test.

Now for the specific TODOs:

• Yes I believe that is a typo that should be fixed
• Unfortunately I don't believe verifyWarpDeriv will return the status of the check, so there is no way to fail the test if this fails. It's possible to fix this to return a status from the Fortran layer, but it will require work. Again, I would say this is an optional enhancement but maybe document this in a general "Testing enhancement" issue for future reference
• If we are okay with updating the reference values once in a while, we could remove the default options and just use what's in IDWarp. Maybe for stability reasons we may not want that, I don't feel strongly about this so I'll let others chime in.

[joanibal]

Why do the parallel test give such different values for "sum of dxs"?
For the O-mesh the values only match to one digit.

[joanibal]

Regarding the attribute error.
Parameterized removes the test methods from the base class, but leaves the others. So when testflo looks for all the test methods, the baseclass will be skipped. However, when testflo looks for all the classes with training methods, the baseclass isn't skipped.

In the adflow tests the train method checks if the class has a name attribute before training since parameterized adds a name attribute to the derived classes.
https://github.com/mdolab/adflow/blob/b3e6d6b47c062c5936e2cff10af6ccd1d57e1fb7/tests/reg_tests/reg_test_classes.py#L25

[ewu63]

Why do the parallel test give such different values for "sum of dxs"?
For the O-mesh the values only match to one digit.

I believe the old tests also had different values for sum of dxs between serial and parallel, so I assumed this was correct. I am not sure why though.

[joanibal]

Using the RegTest class from the adflow tests would automatically add a training method to each of the tests with the necessary condition to catch the attribute error mentioned above. I think this would be useful here, but would probably require refactoring BaseRegTest to include RegTest.

[joanibal]

Why do the parallel test give such different values for "sum of dxs"?
For the O-mesh the values only match to one digit.

I believe the old tests also had different values for sum of dxs between serial and parallel, so I assumed this was correct. I am not sure why though.

Hmm. I can't think of a good reason why those value should be that different.

@eirikurj, @friedenhe, and or @camader , Is it expected that the derivative values for idwarp would be different when run in parallel?

[marcomangano]

A few general comments

* I personally would prefer a single reference file rather than one for serial and one for parallel, since some values are the same. This can also make it more explicit if the serial/parallel values do not mach when they are expected to match. However, I'm not sure exactly how to implement this for training, since you would have incomplete information when you train with just serial tests for example. Maybe we should shelf this and make it an enhancement in the `BaseRegTest` class. What do others think about this?

* Regarding the `AttributeError` during training, is this the same problem that @joanibal ran into for ADflow? What exactly is causing this problem? I wonder if there is a way to handle this without using the hacky solution

* How do you plan to add the complex tests? will it be a separate file, or will you use the conditional import approach? i.e. import either `idwarp` or `idwarp_cs` based on the test.

Now for the specific TODOs:

* Yes I believe that is a typo that should be fixed

* Unfortunately I don't believe `verifyWarpDeriv` will return the status of the check, so there is no way to fail the test if this fails. It's possible to fix this to return a status from the Fortran layer, but it will require work. Again, I would say this is an optional enhancement but maybe document this in a general "Testing enhancement" issue for future reference

* If we are okay with updating the reference values once in a while, we could remove the default options and just use what's in IDWarp. Maybe for stability reasons we may not want that, I don't feel strongly about this so I'll let others chime in.

1. Yes, I would prefer that too but I was getting a bit confused on how to update a currently existing .ref file using BaseRegTests, could not make it work the other way round
2. @joanibal I used your PR on ADflow to get parameterized working but as I was reverse engineering the code I did not have a clear understanding on how to transfer the trick with the "name" attribute here.. maybe we can have a chat offline? I also think that "embedding" the training method used in ADflow in BaseRegTests could be valuable, but I am not sure about the priority level of this potential refactoring.
3. I did not think about it already as I have never run complexified codes on my local machine, I will work on them in the next few days and get back to you. I believe a conditional-import approach is preferred?
4. Done!
5. That's what I got as well, nothing of that is emerging at the python level. Let's wait for other feedback, but I think we can open this issue and see if anybody wants to play with that some time in the future.
6. That's the feeling I got as well, waiting to hear from the maintainers about this.

[joanibal]

I agree more refactoring would be nice but is not necessary for this PR.

When point 3, the complexified tests, is addressed I'll give this PR the green light.

[ewu63]

Firstly, why did you update the refs?

Secondly, some comments on the complex tests. The way you have it set up now will run all tests together, which is a problem since you are stuck with a single PETSc architecture for a single run. So, there are two solutions here:

• ADflow splits everything into separate test functions. That way, you can run testflo -m *complex_test* . to run all complex tests after setting the correct PETSC_ARCH, while reserving the default testflo -m "test* for just real tests.

2. Figure out a way to switch PETSC_ARCH during Python execution. This is possible and has been done before, but will require some work to get working again. I think in the long term, we want to aim for this, such that testflo . should run all possible tests, but skip the complex tests if no complex PETSc installation exists.

What do others think? @joanibal @eirikurj @anily

[marcomangano]

I updated the tests because I did not re-train them after I fixed the bug.

[ewu63]

Thanks @marcomangano for updating the tests. I can verify that now both real and complex tests run, and the skipTest is working as expected.

@joanibal this is something we may want to do for other repos that have complex tests. A single test command testflo . will run real tests, and if the complex library exists, run the complex tests as well. We may want to generalize this further by defining a decorator that does this in BaseRegTest.