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

Improve stats gathering of mapping-tester #210

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented Oct 25, 2024

Main changes of this PR

This PR improves the gathering of stats in the mapping-tester by:

  1. Making it more robust. Various files are now only loaded if they exist. So, post-processing is now optional.
  2. Loads cases in parallel, which can help to improve the import speed of events via precice-profiling

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 force-pushed the improve-stats-gathering branch from 57c69c1 to ab0a415 Compare October 25, 2024 11:27
@fsimonis fsimonis mentioned this pull request Oct 25, 2024
4 tasks
@fsimonis fsimonis mentioned this pull request Nov 8, 2024
5 tasks
@fsimonis fsimonis self-assigned this Dec 10, 2024
@fsimonis fsimonis requested a review from davidscn December 10, 2024 10:04
@fsimonis fsimonis requested a review from davidscn December 11, 2024 10:07
@fsimonis fsimonis force-pushed the improve-stats-gathering branch from ab0a415 to 22ff0f7 Compare January 28, 2025 09:20
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.

I face the following error using the run script in the mapping_tester, not sure if this is related to the changes here

+ python3 ~/precice/aste/examples/mapping_tester/../../tools/mapping-tester//compare.py reference-statistics.csv test-statistics.csv
Comparing results with atol=1e-08 and rtol=1e-05
Given test-statistics.csv
Reference reference-statistics.csv
Traceback (most recent call last):
  File "~/precice/aste/examples/mapping_tester/../../tools/mapping-tester//compare.py", line 63, in <module>
    if not ref.ne_missing(a).any():
           ^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/polars/series/series.py", line 929, in ne_missing
    return self.to_frame().select(F.col(self.name).ne_missing(other)).to_series()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/polars/dataframe/frame.py", line 8097, in select
    return self.lazy().select(*exprs, **named_exprs).collect(_eager=True)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/polars/lazyframe/frame.py", line 1730, in collect
    return wrap_df(ldf.collect())
                   ^^^^^^^^^^^^^
polars.exceptions.ComputeError: cannot evaluate two Series of different lengths (2 and 3)

Error originated in expression: '[(col("median(abs)")) !=v (Series[median(abs)])]'

@fsimonis
Copy link
Member Author

fsimonis commented Jan 28, 2025

Not a pretty error message. I think the case directory still includes the NP mapping case which was removed by #212. There should only be 2 data lines in the file.

@fsimonis fsimonis requested a review from davidscn January 28, 2025 14:30
@davidscn
Copy link
Member

Ah alright, I cleaned it and ran it again:

Part of the mapping-tester is also the plotting script. Using the plotting here gives me

  File "~/precice/aste/tools/mapping-tester/plotconv.py", line 203, in <module>
    sys.exit(main(sys.argv))
             ^^^^^^^^^^^^^^
  File "~/precice/aste/tools/mapping-tester/plotconv.py", line 187, in main
    df = pandas.read_csv(args.file)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 1026, in read_csv
    return _read(filepath_or_buffer, kwds)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 626, in _read
    return parser.read(nrows)
           ^^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/pandas/io/parsers/readers.py", line 1923, in read
    ) = self._engine.read(  # type: ignore[attr-defined]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/precice/precice-venv/lib/python3.12/site-packages/pandas/io/parsers/c_parser_wrapper.py", line 234, in read
    chunks = self._reader.read_low_memory(nrows)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "parsers.pyx", line 838, in pandas._libs.parsers.TextReader.read_low_memory
  File "parsers.pyx", line 905, in pandas._libs.parsers.TextReader._read_rows
  File "parsers.pyx", line 874, in pandas._libs.parsers.TextReader._tokenize_rows
  File "parsers.pyx", line 891, in pandas._libs.parsers.TextReader._check_tokenize_status
  File "parsers.pyx", line 2061, in pandas._libs.parsers.raise_parser_error
pandas.errors.ParserError: Error tokenizing data. C error: Expected 1 fields in line 3, saw 2

@davidscn
Copy link
Member

Maybe we should add the plotconv to the testing pipeline (I thought it was actually part of it)?

@fsimonis
Copy link
Member Author

fsimonis commented Jan 28, 2025

True. Let's do that in a separate issue though.

The plotting scripts rely on the meshes being numerical values for the x axis, which currently doesn't work with our examples.

This crash looks very wild.

@davidscn
Copy link
Member

True. Let's do that in a separate issue though.

The plotting scripts rely on the meshes being numerical values for the x axis, which currently doesn't work with our examples.

This crash looks very wild.

The problem is a bit that the current changes lead to the wild crash. So if I accept the changes, the script doesn't work anymore. The issue is an immediate consequence of the changes being made here. What do you think?

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.

2 participants