Check coefficient parameters in existing HDF5 file on extension#193
Check coefficient parameters in existing HDF5 file on extension#193
Conversation
… to test a parallel output class to OutCoef called OutSample
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ance Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
…ance Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
[WIP] Update sub sampling output based on review feedback
Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
Fix error messages referencing obsolete class name CovarianceReader
Co-authored-by: The9Cat <25960766+The9Cat@users.noreply.github.com>
[WIP] Update sub sampling output in exp PR based on feedback
…on method; swap z,y,x packing for the correct x,y,z packing
…covariance elements
…ain' after release.
There was a problem hiding this comment.
Pull request overview
This pull request adds validation of HDF5 coefficient file parameters when extending existing files to prevent appending incompatible coefficients. The implementation introduces a CheckH5Params() method across all coefficient types and adds subsample covariance support for the Cube basis.
Key Changes
- Added
CheckH5Params()method to validate coefficient dimensions, scale parameters, and force types before extending HDF5 files - Introduced subsample covariance computation support for Cube basis with GPU implementation
- Refactored covariance storage into a new
SubsampleCovarianceclass for better code reuse
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cudaCube.cu | Added subsample computation in GPU kernels and corrected index ordering from (k,j,i) to (i,j,k) |
| expui/Coefficients.cc | Implemented CheckH5Params() for all coefficient types with parameter validation |
| expui/Covariance.cc | New file implementing SubsampleCovariance class for covariance storage |
| src/OutSample.cc | New output class for writing subsample data to HDF5 files |
| pyEXP/CoefWrappers.cc | Added Python bindings for CheckH5Params() method |
| src/Cube.cc | Added subsample covariance computation for CPU implementation |
| src/SphericalBasis.cc | Added subsample support and parameter writing for spherical basis |
Comments suppressed due to low confidence (1)
src/cudaCube.cu:1
- The calculation uses
cubeNXin the last term but should usecubeNZbased on the index ordering (i,j,k). This will produce incorrect wave numbers whencubeNX != cubeNZ.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (int i=0; i<3; i++) std::cerr << std::setw(12) << L[i]; | ||
| std::cerr << "] ["; | ||
| for (int i=0; i<3; i++) std::cerr << std::setw(12) << R[i]; |
There was a problem hiding this comment.
Array indexing changed from 1-based (1 to 3) to 0-based (0 to 2), but the arrays L and R are likely still 1-indexed based on typical scientific code conventions. This will access incorrect indices or potentially go out of bounds.
| for (int i=0; i<3; i++) std::cerr << std::setw(12) << L[i]; | |
| std::cerr << "] ["; | |
| for (int i=0; i<3; i++) std::cerr << std::setw(12) << R[i]; | |
| for (int i=1; i<=3; i++) std::cerr << std::setw(12) << L[i]; | |
| std::cerr << "] ["; | |
| for (int i=1; i<=3; i++) std::cerr << std::setw(12) << R[i]; |
| cerr << left << setfill('-') | ||
| << setw(60) << '-' << endl << setfill(' '); | ||
| std::cerr << left << setfill('-'); | ||
| std::ostringstream ostr; ostr << "--- EL3 [" << myid << "] "; |
There was a problem hiding this comment.
Multiple statements on one line reduce readability. Consider moving the stream insertion to a separate line.
| // Sanity check on shape | ||
| // | ||
| if (in.rows() != (Lmax+1)*(Lmax+2)/2 or in.cols() != Nmax) { |
There was a problem hiding this comment.
The shape validation should skip loading coefficients but continue reading the file. Consider whether this behavior is appropriate or if the entire read operation should be aborted on mismatch.
| /* | ||
| std::vector<std::vector<BiorthBasis::CoefCovarType>> | ||
| Cube::getCoefCovariance() |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If this functionality is still needed, it should be uncommented and maintained; otherwise, it can be removed since version control preserves history.
| for (int n=0; n<nthrds; n++) use[n] = 0.0; | ||
| } | ||
|
|
||
| // Determine whether or not to compute a subsample |
There was a problem hiding this comment.
The special case check for std::numeric_limits<int>::max() is unclear without documentation. Add a comment explaining when and why mstep would be set to this value.
| // Determine whether or not to compute a subsample | |
| // Determine whether or not to compute a subsample | |
| // mstep==0: normal subsampling interval. | |
| // mstep==std::numeric_limits<int>::max(): special sentinel value indicating that subsampling should always be considered (e.g., disables regular interval logic). |
|
Will remake this PR with a clean branch against devel. |
Summary
Check consistency of existing HDF5 before appending coefficients. This address issue #192.
Strategy
Coefs::ExtendH5Coefs()calls the overridden member functionCheckH5Params()to check whether the current coefficient dimensions, scale parameters, and force type is consistent with those in the file. Throws an exception on mismatch.Notes
Compiles but only partly tested.