-
Notifications
You must be signed in to change notification settings - Fork 10
Check coefficient parameters in existing HDF5 file on extension #193
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]; |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 << "] "; |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.