search

precice / systemtests

Testing preCICE / solver combinations using Docker
GNU General Public License v3.0
3 stars 4 forks source link

Is vtk reference output for tests/TestCompose_fe-fe useful? #267

Open BenjaminRodenberg opened 3 years ago

BenjaminRodenberg commented 3 years ago

I'm not sure whether the vtk reference output for the test tests/TestCompse_fe-fe is required or not. Note that it has to be maintained (#266). I know that this case is a bit exotic, since it has an analytical solution and we check for the error in every timestep. But exactly this is also the reason, why I get the impression that we can save some (unnecessary?) work by getting rid of the vtk reference output for this case.

Any opinions on this? Or is this a standard that we follow for all systemtests? "Every systemtest stores and compares the vtk files at the interface". Making an exception just for this one test will not help us in the end. In this case I can also live with the need for some additional maintenance.

MakisH commented 3 years ago

We recently had several cases where we had to constantly update the comparison scripts to consider the format of each new solver/adapter, and especially to ignore time-specific lines and any new output of each adapter.

We recently applied (https://github.com/precice/systemtests/issues/252) a new scheme where in the system tests we only check the preCICE VTK exports. So, these files are new and the purpose is to only look at a subset of files that are easier to check and maintain.

Before you ask: we can still check every missing comma regression in the output of an adapter in the CI of each adapter. We want to avoid this complication in the full-scale system tests.

IshaanDesai commented 3 years ago

Something related to this discussion. There was a slight variation in the preCICE VTK output generated on a local machine and the output generated via the CI pipeline. This prompted a need to update the reference output in a different way and is described here: https://github.com/precice/systemtests/pull/268

MakisH commented 3 years ago

This is a reproducibility issue that we should address in the long term over all the components that we control. Even a different compiler version / optimization level could cause small differences (e.g. due to order of evaluation).

Did you generate the results in your own system or running the system tests locally?

IshaanDesai commented 3 years ago

Did you generate the results in your own system or running the system tests locally?

I generated the results by triggering the system test remotely (the CI pipeline) and made sure the VTK output is dumped in precice_st_output repository using python push.py -o command. I then used this output to update the reference results.

MakisH commented 3 years ago

Alright, then this is going to a different direction.

@BenjaminRueth @IshaanDesai please close this if the question is answered.

IshaanDesai commented 3 years ago

Alright, then this is going to a different direction.

To come back to the main discussion point in this issue, My opinion now is that we keep the VTK output because it is a standardization for all tests and makes the fe-fe case even more bullet proof. The comparison with analytical solution is done within the case itself and additionally the VTK is now compared. This is a lot of checking yes, but nothing wrong in that! @BenjaminRueth do you agree?

MakisH commented 3 years ago

ok, I see your point now: the fe-fe case is different because you also check for correctness internally. In that case, we don't even need this in the system tests, only in the FEnICS adapter CI, right? Do we check anything else in the system tests that we don't check in the adapter tests?

Having the case in the system tests but excluding it from the comparisons is difficult (to maintain). Removing the case all together is easier.

IshaanDesai commented 3 years ago

In that case, we don't even need this in the system tests, only in the FEnICS adapter CI, right? Do we check anything else in the system tests that we don't check in the adapter tests?

I agree that we can move this test to the FEniCS-Adapter CI, mainly because there is no dependency of any other solver or solver-adapter. AFAIK the system tests do not check anything else that the adapter tests dont check.

BenjaminRodenberg commented 3 years ago

We now have a basic version of the HT test in the fenics-adapter repository. Note that it is still not "complete" see https://github.com/precice/fenics-adapter/pull/115#issuecomment-757832839 and https://github.com/precice/fenics-adapter/issues/116. If we come to a conclusion under https://github.com/precice/fenics-adapter/issues/116 I can take care of implementing everything needed.