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 mapping tester reference results #212

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

fsimonis
Copy link
Member

Main changes of this PR

This PR adds a mapping tester compare script and reference results.

The script checks:

  • Existing columns
  • Equal test cases
  • Valid time and memory data (>0) as comparing is difficult across machines
  • Approx identical mapping results

Closes #208

Author's checklist

  • I used the pre-commit hook and used pre-commit run --all to apply all available hooks.
  • I added a test to cover the proposed changes in our test suite.
  • I updated the documentation in docs/README.md.
  • I added a changelog entry in ./changelog-entries/ (create if necessary).
  • I updated potential breaking changes in the tutorial precice/tutorials/aste-turbine.

@fsimonis fsimonis requested a review from davidscn October 25, 2024 15:26
Copy link
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great step for more robust testing. Currently, tests are failing.

import argparse
from sys import exit

import polars as pl
Copy link
Member

@davidscn davidscn Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a problem with polars being called via ctest: in my venv it works locally (using run.sh), but ctest fails for me in the same bash session.

Copy link
Member Author

@fsimonis fsimonis Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A problem is that we currently overwrite the PATH at configuration time. So any further modifications are not picked up.

I just saw that CMake 3.22 (our new baseline) provide ENV modification support for exact this use-case. https://cmake.org/cmake/help/v3.22/prop_test/ENVIRONMENT_MODIFICATION.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering why it worked or works for the precice-profiling script (also used in the mapping tester), whereas it fails for the compare script. The precice-profiliing script is called through the gathering script os.subprocess, maybe that's the simple answer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my venv, the tests pass now. The CI doesn't seem to be happy

https://github.com/precice/aste/actions/runs/11552451140/job/32151545870?pr=212#step:10:447

I am not sure if the actual solution here would be more simple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+ python3 /Users/runner/work/aste/aste/examples/mapping_tester/../../tools/mapping-tester//compare.py reference-statistics.csv test-statistics.csv
The test results differ in both files
DataFrames are different (value mismatch for column 'median(abs)')
[left]:  [0.00565710724966223, 0.0004046891096261551]
[right]: [0.00567422622697189, 0.00040468910964214233]

Isn't this a property from the mapping accuracy that should be unchanged?

The polars comparision uses rtol of 1e-05 and an atol of 1e-08.

https://docs.pola.rs/api/python/stable/reference/api/polars.testing.assert_frame_equal.html#polars.testing.assert_frame_equal

Copy link
Member Author

@fsimonis fsimonis Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASTE generates, exports, partitions to VTU with binary data and uses the VTK-internal calculator.

The only moving targets are the VTK version and the numpy version, which we use to calculate the percentiles/median.

Maybe they changed the interpolation method? Still uses linear interpolation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert checks col by col and the first column is the median. So it is possible that other columns don't match too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have the complete result table for both runs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be a matter of the column order or something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a morning of debugging we realised that the partitioning wasn't deterministic, which lead to major differences in NP and even tiny run to run differences in RBF TPS.
We never faced this issue before as we are now using separate cases, compared to
separate runs. Using topology-based partitioning solves the issue and leads to reproducible cases.

@fsimonis fsimonis force-pushed the add-mapping-tester-compare-script branch from ac9c3ca to 91f154d Compare October 28, 2024 15:58
@@ -1,4 +1,5 @@
jinja2
numpy
polars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would require a documentation update.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is primarily to get the CI working. I'll revert later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it will remain a requirement to execute the tests, right? In the past, we also had tests for the mapping-tester here, which has optional dependencies. Then, however, users install the software, run ctest and see that tests are failing.
How does this behave here then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the mapping-tester-test needs precice-profiling analyze which requires polars anyway. So the dependency is already there.

If we merge #210, then the timings and thus the dependency on polars becomes optional.
In that case, it may be an idea to implement the compare without polars.

On the flip side, this then becomes a bad test, as it tests two different things depending on the environment. Maybe a warning in CMake is enough here.

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

Successfully merging this pull request may close these issues.

Test mapping-tester output in CI
2 participants