Add test for writing and reading Legendre polynomials from disk#413
Add test for writing and reading Legendre polynomials from disk#413samhatfield wants to merge 20 commits into
Conversation
|
The history of this read/write was for the serial postprocessing of operational (high) resolution using following stack hierarchy: |
|
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 |
There was a problem hiding this comment.
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_transAPI test that writes Legendre polynomials to disk and then reads them back in a secondSETUP_TRANScall. - Extend Legendre polynomial file headers (CPU/GPU) to include
IRBYTESand 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.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
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? |
6fb498d to
6a7728e
Compare
Correct, they're no longer backwards compatible. I've added 4 bytes to the header for storing the packed version integer, and two extra integers in case of future changes. Look good to you? |
|
Hi @samhatfield what I meant with version is the version of the file format; not necessarily the ectrans version, although that is also good to know... The file format version only gets bumped when we actually change something to the file format. We can set it to If we're to design this really good, and we have the chance now, I suggest the header to contain this:
The ordering of 4..8 does not matter, but what's here and was already here is a good choice. Finally, after all the data is written, I'd also add another string, "ECTRANS_LEGPOL_END" Then when reading in the file back we need to add assertions:
Then we can further read in the rest safely. Finally verify that we encounter "ECTRANS_LEGPOL_END" |
|
All good ideas. Let me take a look. |
ad52119 to
92581d1
Compare
|
@wdeconinck the header now looks like the following: I'm not that familiar with BOZ literals in Fortran so if you could take a look at how I've handled the BOM and whether I've done it correctly, that would be appreciated... |
|
It's nice to have a HEADER_END marker; it does not necessarily have to be a large string. |
There's already an end marker
I've added this functionality to |
|
Great about the EoF already. The capability of doing the rewind or skip is not necessary now but is good to have in the file format if possible.
Sent from Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Sam Hatfield ***@***.***>
Sent: Thursday, 25 June 2026 11:55:41
To: ecmwf-ifs/ectrans ***@***.***>
Cc: Willem Deconinck ***@***.***>; Mention ***@***.***>
Subject: Re: [ecmwf-ifs/ectrans] Add test for writing and reading Legendre polynomials from disk (PR #413)
[https://avatars.githubusercontent.com/u/8796885?s=20&v=4]samhatfield left a comment (ecmwf-ifs/ectrans#413)<#413 (comment)>
It's nice to have a HEADER_END marker; it does not necessarily have to be a large string. I would use ECTRANS_LEGPOL_FINAL at the very end of the file (after writing all the actual data); This can be used to check if a file was incomplete.
There's already an end marker 'LEGPOL---EOF-EOF' which is checked in READ_LEGPOL.
What would also be nice, if possible to add to the header is the number of bytes in the data section, following the HEADER_END section all the way upto (not including) ECTRANS_LEGPOL_FINAL. So some kind of precomputation of expected bytes... That could make it possible to read all the data into memory in advance, and also allow to file-jump to the ECTRANS_LEGPOL_FINAL string to verify the file is complete.
I've added this functionality to WRITE_LEGPOL, but it isn't currently used in READ_LEGPOL. I'm not sure how to skip ahead by a prescribed number of bytes like you suggest. Does BYTES_IO_WRITE increment an offset from the start of the file? Do I just need to read a dummy buffer of NBYTES and then check that the next n bytes matches the expected end marker ('LEGPOL---EOF-EOF')? I suppose I would then need to rewind to finish reading the header.
—
Reply to this email directly, view it on GitHub<#413?email_source=notifications&email_token=AABWNZ7JBERXWWGACSZYGET5BTZJ3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINZZG44TSMBSGA32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4797990207>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABWNZZHYKOSQLD75WFJVVT5BTZJ3AVCNFSNUABFKJSXA33TNF2G64TZHM2DENRVG42DSMJTHNEXG43VMU5TINRSGIZDMNRUGM4KC5QC>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I checked whether the precomputed file size matches the actual size for the test, and it does: Single precision Double precision |
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_TRANSwith disk-read polynomials. This PR adds a test which callsSETUP_TRANSonce 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 callingSETUP_TRANS?Contributor Declaration
By opening this pull request, I affirm the following: