Skip to content

Fixes and unit tests for Field3DParallel#3428

Open
dschwoerer wants to merge 13 commits into
nextfrom
f3dp-fixes-tests
Open

Fixes and unit tests for Field3DParallel#3428
dschwoerer wants to merge 13 commits into
nextfrom
f3dp-fixes-tests

Conversation

@dschwoerer

Copy link
Copy Markdown
Contributor

The templated expression broke a significant bit of the Field3DParallel implementation.

This forces to to not use the templates for Field3DParallel. It might be nice to fix them instead, but that takes more effort.

It also adds unit tests to avoid future regressions.

@dschwoerer dschwoerer requested a review from ZedThree July 9, 2026 11:07

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment thread tests/unit/fake_mesh.hxx
Comment thread tests/unit/fake_mesh.hxx
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
Comment thread tests/unit/fake_mesh.hxx Outdated
@bendudson

Copy link
Copy Markdown
Contributor

Thanks for the tests @dschwoerer ! I've almost finished making Field3DParallel expressions lazy templates. I'll use the tests to catch regressions.

@ZedThree ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, comments don't really need to be fixed

EXPECT_TRUE(IsFieldEqual(prod, 6.0));

const Field3DParallel prodpar = field.asField3DParallel() * rhs;
EXPECT_TRUE((std::is_same_v<std::decay_t<decltype(prodpar)>, Field3DParallel>));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::is_same_v is a compile-time thing, so this will always be true

Comment thread include/bout/field3d.hxx
Comment on lines 784 to +786
ASSERT1_EXPR_COMPATIBLE(lhs, rhs); \
static_assert(!std::is_same_v<L, Field3DParallel>); \
static_assert(!std::is_same_v<R, Field3DParallel>); \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concepts might help here

@bendudson

Copy link
Copy Markdown
Contributor

Please hold off merging this for now. #3430 is an alternative that (I hope) will get the desired behavior while integrating Field3DParallel types into the template expressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants