Skip to content

Add test for writing and reading Legendre polynomials from disk#413

Open
samhatfield wants to merge 14 commits into
developfrom
feat/add_legpoly_read_test
Open

Add test for writing and reading Legendre polynomials from disk#413
samhatfield wants to merge 14 commits into
developfrom
feat/add_legpoly_read_test

Conversation

@samhatfield

Copy link
Copy Markdown
Collaborator

Before merging #409 I want to add a test for doing a direct transform with Legendre polynomials read from disk. Firstly I want to check whether we can call SETUP_TRANS with disk-read polynomials. This PR adds a test which calls SETUP_TRANS once to write the polynomials to disk and then again in the same instance to read the polynomials from the same file. However, the test currently segfaults which either indicates a bug in the code or improper use. @wdeconinck can you see anything wrong with how I'm calling SETUP_TRANS?

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

@wdeconinck

wdeconinck commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

The history of this read/write was for the serial postprocessing of operational (high) resolution using following stack hierarchy:
MARS -> MIR -> Atlas -> transi -> trans
The Legendre coefficients could then be cached, or precomputed. For operational resolution these are quite big, and computation in serial is too expensive. In our parallel IFS context the recomputation is not an issue making the caching an extra complexity.
All this still comes from the time when trans was still part of the IFS and transi a standalone repository.
As such I had created a unit-test in transi to exercise this: https://github.com/ecmwf-ifs/ectrans/blob/develop/tests/transi/transi_test_io.c
It may serve as hints on how to do this just with Fortran.
Although we don't really exercise this in any Fortran context, it is still a useful Fortran test to add.
Note this is all in a serial context (NPROC==1) only.

@samhatfield

Copy link
Copy Markdown
Collaborator Author

It was just a simple precision bug -> the writer and reader assumed 8-byte reals, always, so the single-precision tests failed. I've fixed this by adjusting the type of the polynomials based on JPRB and embedding this in the legpol file metadata.

@samhatfield samhatfield requested a review from Copilot June 11, 2026 12:14
@samhatfield samhatfield marked this pull request as ready for review June 11, 2026 12:14
@samhatfield samhatfield requested a review from wdeconinck June 11, 2026 12:14
@github-actions github-actions Bot requested a review from marsdeno June 11, 2026 12:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds an API test intended to validate that SETUP_TRANS can (1) write Legendre polynomials to disk and (2) re-initialize in the same process reading those polynomials back from the same file. To support this, it also extends the Legendre polynomial on-disk header with the real-element byte size (IRBYTES) in both CPU and GPU implementations.

Changes:

  • Add a new setup_trans API test that writes Legendre polynomials to disk and then reads them back in a second SETUP_TRANS call.
  • Extend Legendre polynomial file headers (CPU/GPU) to include IRBYTES and use it when sizing binary reads/writes.
  • Update CMake test list/excludes to register the new test and skip it for MPI>0 configurations.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/trans/api/setup_trans/setup_trans_test_suite.F90 Adds the new write/read Legendre polynomial test case.
tests/trans/api/setup_trans/CMakeLists.txt Registers the new test and excludes it for MPI configurations.
src/trans/gpu/internal/write_legpol_mod.F90 Writes extended Legendre header including IRBYTES (GPU path).
src/trans/gpu/internal/read_legpol_mod.F90 Reads extended Legendre header including IRBYTES (GPU path).
src/trans/cpu/internal/write_legpol_mod.F90 Writes extended Legendre header including IRBYTES (CPU path).
src/trans/cpu/internal/read_legpol_mod.F90 Reads extended Legendre header including IRBYTES (CPU path).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/trans/gpu/internal/write_legpol_mod.F90 Outdated
Comment thread src/trans/gpu/internal/read_legpol_mod.F90 Outdated
Comment thread src/trans/gpu/internal/read_legpol_mod.F90 Outdated
Comment thread src/trans/cpu/internal/read_legpol_mod.F90 Outdated
Comment thread tests/trans/api/setup_trans/setup_trans_test_suite.F90 Outdated
@wdeconinck

Copy link
Copy Markdown
Collaborator

Due to the extra entry in HEADER, would you say that generated files from before this PR are no longer compatible to be read in?
If no longer backwards compatible, I would add reserve some extra bytes padded into the header that could possibly be filled later.
I would also add a version number which can be checked for compatibility.
It is important to try to be backwards compatible for the future, so that caching mechanisms don't need to adapt.
It is probably OK to break backwards compatibility at this time because MARS/MIR has moved away from using this, in favour of an Atlas-serial implementation for arbitrary grids.

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.

4 participants