Repository navigation
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (46.60%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #14585 +/- ##
===========================================
- Coverage 77.58% 77.46% -0.11%
===========================================
Files 2962 2903 -59
Lines 512659 490414 -22245
Branches 79368 74227 -5141
===========================================
- Hits 397713 379899 -17814
+ Misses 86736 85590 -1146
+ Partials 28210 24925 -3285
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Abuudiii
approved these changes
Oct 30, 2026
|
|
||
| const auto counterpart = layout.get_other_inplace_layout_for(other_io, fft_type, odd); | ||
| ASSERT_EQ(counterpart.has_value(), follows_rules); | ||
| if(counterpart) |
Contributor
There was a problem hiding this comment.
Should you also be able to ASSERT_EQ(counterpart) at this point?
Contributor
Author
There was a problem hiding this comment.
counterpart has no value whenever the layout breaks the rules (roughly half of the random cases): get_other_inplace_layout_for is documented to return a value iff an in-place counterpart exists. So the ASSERT_EQ above checks that part of the contract, and the if then checks the derived layout only when there is one.
| @@ -570,6 +570,13 @@ TEST_F(data_layout_test, inplace_real_round_trip) | |||
| // operator== ignores the embedding map, so compare it separately | |||
| EXPECT_TRUE(back->get_embedding_layout() == full) << back->str(); | |||
|
|
|||
| // a Hermitian innermost length of 1 only comes from a real one of 1 | |||
Contributor
There was a problem hiding this comment.
Is L[0]==1 excluded here?
Contributor
Author
There was a problem hiding this comment.
No, L[0] == 1 goes through the full round trip above. Any Hermitian innermost length get_other_inplace_layout_for now rejects it with std::invalid_argument, as documented in the last commit, and the test expects that throw instead.
Contributor
|
No concerns with this PR, but I had a couple of questions/clarifications. |
regan-amd
force-pushed
the
users/regan-amd/rocfft-data-layout-test-gaps
branch
from
November 3, 2026 17:34
112a5db to
7ce1359
Compare
rocfft-internal-test now accepts --num_random_cases (default 200, must be positive) next to --seed, and the randomized data_set tests use it instead of a hard-coded constant. A failing case replays with the same --seed and at least as many --num_random_cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Several data_set_t queries could be broken without failing a test, because the tests never generated unit-length axes (e.g. a batch of 1), or checked fixed layouts only: - offset_in: drop the min_span parameter of random_full_range so its random layouts include unit-length axes, and check that layouts with the same total number of axes but a different length/batch split are rejected. - continuous_chunk: draw spans (some of them 1) in random axis order with random padding, so that the slowest-varying axis is not always the last one; removing the unit-length skip in slowest_varying_axis() used to pass. Also check that a chunk running past the slowest-varying axis is rejected and that strides along unit-length axes are ignored. - same_logical_range: new test, has_same_logical_range_as was not called by any test. - contiguous_strides: randomize, deriving the expected contiguity from the construction; an is_contiguous() ignoring batch axes used to pass. - intersection: check that different length/batch splits and different embeddings are rejected, and that a shared embedding is kept. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- inplace_complex_is_unchanged only checked a packed 1-D layout for a forward transform's output. Draw random layouts that may be partial along batch axes and embedded, with one transform direction and I/O side per case, and compare embedding maps separately since operator== ignores them. - inplace_real_round_trip: multi-device plans also derive in-place layouts of embedded views that keep the embedding's innermost axis (see plan.cpp). Run the round trip on such views, and check that the Hermitian side of a view is the view of the Hermitian side of the whole: this catches strides of former length axes (now batch axes) not being rescaled, which the round trip alone cannot. - inplace_real_refuses_impossible_layouts checked one rule at a time on a 1-D layout, for a forward transform's real side only. Rename it inplace_real_counterpart_rules and build random layouts by nesting their axes, with unit-length axes, odd strides and strides around the padded-row threshold, for either transform direction and either side, checking both acceptance and the derived layout. The layouts are never self-aliased, and neither are their counterparts when the per-axis stride rules hold, so the expected acceptance stays valid once the library detects self-aliasing; until then it restates the library's rules, which its comment documents as a known weakness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
data_set_t::get_other_inplace_data_set_for returned a real layout with an innermost length of 0 when asked for the real side of a Hermitian innermost length of 1 with other_innermost_length_is_odd set to false. A Hermitian length of 1 can only come from a real length of 1, so throw std::invalid_argument instead, and document it. Callers in plan.cpp compute the flag from actual lengths and are not affected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
regan-amd
force-pushed
the
users/regan-amd/rocfft-data-layout-test-gaps
branch
from
November 3, 2026 22:35
7ce1359 to
a79f268
Compare
Follow-up to #14063, which renamed data_layout_t to data_set_t: several comments and error messages in data_set.{h,cpp} still called data_set_t objects layouts, e.g. get_other_inplace_data_set_for was documented to return the "corresponding output (resp. input) layout". Call them data sets, and keep "layout" where it refers to the in-buffer arrangement of the data (e.g. "a contiguous data layout"), to the public rocfft_plan_description_set_data_layout, or to the layout members of descriptions and bricks that #14063 leaves for a separate rename. No functional change beyond the wording of error messages.
Following #14063 and the previous commit, rename the data_set_test.cpp variables and trace labels that still called data_set_t objects layouts (layout, layout_1d, layout_2d, ...) and reword the matching comments. "layout" is kept for client_data_layout_helpers.h and the test comparing against it.
regan-amd
changed the title
evetsso
self-requested a review
November 4, 2026 20:02
evetsso
approved these changes
Nov 4, 2026
1 task done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
JIRA ID : AIFFT-543
Follow-up to #12667. Deliberately breaking
data_set.cppshowed several bugs that the mergeddata_set_ttests could not catch, mostly because they never generated unit-length axes (e.g. a batch of 1) or embedded views, or only checked a few fixed data sets. Some of those code paths are used by multi-device plans inplan.cpp. Writing the missing tests also turned up a small library issue:get_other_inplace_data_set_forcould return a real data set with an innermost length of 0.This PR is based on #14063 (
data_layout_trenamed todata_set_t) and also addresses wording that the rename left behind.Technical Details
Six commits, best reviewed one at a time:
--num_random_casesoption forrocfft-internal-test(default 200, must be positive), next to--seed. The randomizeddata_settests use it instead of a hard-coded constant; a failing case replays with the same--seedand at least as many--num_random_cases.offset_in: random data sets now include unit-length axes; data sets with the same total number of axes but a different length/batch split are rejected.continuous_chunk: random spans (some of them 1) in random axis order, so the slowest-varying axis is not always the last one; the unit-length skip inslowest_varying_axis()was previously untested.same_logical_range: new;has_same_logical_range_aswas not called by any test.contiguous_strides: randomized, with the expected contiguity derived from how the data set is built.intersection: different length/batch splits and different embeddings are rejected, and a shared embedding is kept in the result.inplace_complex_is_unchanged: random data sets, possibly partial along batch axes and embedded, for either complex direction and either I/O side; embedding maps are compared separately sinceoperator==ignores them.inplace_real_round_trip: also runs on embedded views keeping the embedding's innermost axis, as used by multi-device plans, and checks that the Hermitian side of a view is the view of the Hermitian side of the whole.inplace_real_counterpart_rules(renamed frominplace_real_refuses_impossible_layouts): random data sets built by nesting their axes, for either real direction and either side, checking both acceptance and the derived data set. The data sets are never self-aliased, and neither are their counterparts whenever the per-axis stride rules hold. The test's comment documents that its expected acceptance currently restates the library's per-axis rules (a known weakness that goes away once the library detects self-aliasing).data_set_t::get_other_inplace_data_set_fornow throwsstd::invalid_argumentwhen asked for the real side of a Hermitian innermost length of 1 withother_innermost_length_is_odd = false, which would otherwise give a real innermost length of 0. Current callers inplan.cppcompute that flag from actual lengths and are not affected, so there is no user-visible change. The function's documentation is updated accordingly.data_set.{h,cpp}: comments and error messages that still calleddata_set_tobjects layouts now call them data sets (e.g.get_other_inplace_data_set_forwas documented to return the "corresponding output (resp. input) layout"). "layout" is kept where it refers to the in-buffer arrangement of the data, to the publicrocfft_plan_description_set_data_layout, or to thelayoutmembers of descriptions and bricks that refactor(rocfft): rename data_layout_t to data_set_t #14063 leaves for a separate rename. No functional change beyond the wording of error messages.data_set_test.cpp: variables and trace labels namedlayout(layout_1d,layout_2d, ...) are renamed todata_set, and the matching comments reworded.Known limitation, not addressed here:
get_other_inplace_data_set_forchecks strides one axis at a time, so it can accept a real data set whose Hermitian counterpart is self-aliased (e.g. lengths{8, 2, 2}with strides{1, 10, 18}). Self-aliasing detection is planned separately; the tests do not generate such data sets.The randomized tests use a new random seed on every run by default. On failure, the seed is printed and attached to every assertion message, so the case replays with
--seed <value> --gtest_filter=<test>.Test Plan
Before rebasing onto #14063 (commits 1-4):
librocfftandrocfft-internal-test(Release,-DWERROR=ON,-DROCFFT_BUILD_INTERNAL_TESTS=ON, RCCL on, gfx1201) with amdclang++ from a TheRock build of ROCm 10.3.0 and from ROCm 7.0.2, thenctest, and thedata_layouttests with seeds 0-499 and with--num_random_cases 5000.-Werror) and run with 200-500 seeds; the final tree with seeds 0-1999 (g++) and 0-299 (amdclang++).slowest_varying_axis(), makinghas_same_logical_range_asalways true, ignoring batch axes inoffset_in/is_contiguous(), dropping the embedding map in in-place copies or intersections, not rescaling batch-axis strides on the Hermitian side, relaxing each stride rule of real in-place layouts).After rebasing onto #14063 (commits 1-6):
-Werror);data_set_test.*run with seeds 0-299.rocfft-internal-test(commits 1-4) built with ROCm 7.0.2, thenctest, anddata_set_test.*with seeds 0-499 and with--num_random_cases 5000.librocfftitself could not be built with ROCm 7.0.2 on currentdevelop, independently of this PR (also fails without it), so the full build relies on CI.Test Result
ctest20/20 passed with both ROCm versions; all seed runs pass. Every deliberate bug above makes at least one test fail, including those the merged tests missed; bugs are caught within 20-50 random cases in most cases, and with the default 200 cases per test in at least 99% of runs.ctest25/25 passed; all seed runs pass.Submission Checklist