-
Notifications
You must be signed in to change notification settings - Fork 9
Add tests #179
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
Add tests #179
Conversation
9d5dd77 to
6c859c9
Compare
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 PR refactors the layout transformation system by replacing the abstract test implementation with a concrete PointwiseConcatTransform implementation and comprehensive tests. The changes introduce better error handling through typed return values and add support for field-to-column mapping via an IndexMap structure.
Key Changes:
- Introduces
PointwiseConcatTransformclass that concatenates tensor fields into a single matrix for ML model input - Updates
LayoutTransforminterface to returnAMSExpected<IndexMap>frompack()andAMSStatusfromunpack()for better error handling - Adds comprehensive test coverage with 8 test cases covering both success scenarios and error conditions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/AMSlib/wf/pointwise_layout_transform.cpp |
New test file with 8 test cases covering pack/unpack operations, uncertainty handling, and error conditions |
tests/AMSlib/wf/layout_transform.cpp |
Removed old dummy test implementation |
tests/AMSlib/wf/CMakeLists.txt |
Updated test registration to use new pointwise test suite |
src/AMSlib/wf/pointwise_layout_transform.hpp |
New concrete implementation of LayoutTransform with field concatenation logic |
src/AMSlib/wf/index_map.hpp |
New struct defining field-to-column mappings for tensor transformations |
src/AMSlib/wf/layout_transform.hpp |
Updated interface with typed error returns and reference-based output parameters |
src/AMSlib/include/AMSError.hpp |
Added InvalidShapes error type for tensor dimension mismatches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lpottier
left a comment
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.
LGTM. The concept of LayoutTransform and IndexMap is good but will require exhaustive documentation with examples.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Thank you. I am planning to add more documentation. I will start doing it after I get an end to end example working with this. I am also suspecting I have bugs in my overall logic. So I need this end-to-end example to fix/simplify components. |
No description provided.