Skip to content
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

Allow multiple post interpolation callbacks #5569

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

knelli2
Copy link
Contributor

@knelli2 knelli2 commented Oct 18, 2023

Proposed changes

Doesn't actually add any, but adds support for adding more.

For reviewers, most of the important logic changes are in InterpolationTargetDetail.hpp. The rest is mostly just changing the name of the type alias and wrapping them in a tmpl::list<> .

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

Copy link
Member

@wthrowe wthrowe left a comment

Choose a reason for hiding this comment

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

Also CI is unhappy.

struct AllSimpleCallbacks<tmpl::list<Callbacks...>, Box, Cache, TemporalId>
: public std::integral_constant<
bool, tmpl2::flat_all_v<is_apply_callable_v<Callbacks, Box, Cache,
TemporalId>...>> {};
Copy link
Member

Choose a reason for hiding this comment

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

[optional] You can just inherit from tmpl2::flat_all<...> and skip the type -> bool -> type conversion, since that already inherits from an integral_constant.

As another alternative, you could use a fold expression. I don't know if anyone's compared performance between those and flat_all, but these are small lists so it shouldn't matter.

And yet another alternative: Is

AllSimpleCallbacks<callbacks, decltype(*box), decltype(*cache),
                   decltype(temporal_id)>

sufficiently clearer than

tmpl::all<callbacks,
          is_apply_callable<tmpl::_1, tmpl::pin<decltype(*box)>,
                            tmpl::pin<decltype(*cache)>,
                            tmpl::pin<decltype(temporal_id)>>

to be worth the extra struct?

template <typename Callbacks>
struct get_invalid_fill {
using type =
tmpl::find<tmpl::flatten<tmpl::transform<Callbacks, get_value<tmpl::_1>>>,
Copy link
Member

Choose a reason for hiding this comment

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

What is being flattened here? I think the argument is something along the lines of tmpl::list<NoSuchType, DoubleWrapper<T>>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I have no clue why, but when I wrote this I thought tmpl::flatten did a tmpl::filter instead. I'll just remove it

}
}

CREATE_HAS_STATIC_MEMBER_VARIABLE(fill_invalid_points_with)
CREATE_HAS_STATIC_MEMBER_VARIABLE_V(fill_invalid_points_with)

template <typename T, bool B>
struct with_value;
Copy link
Member

Choose a reason for hiding this comment

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

This namespace isn't super-specific, so name these structs something more descriptive.

struct get_invalid_fill {
using type =
tmpl::find<tmpl::flatten<tmpl::transform<Callbacks, get_value<tmpl::_1>>>,
tt::is_a<DoubleWrapper, tmpl::_1>>;
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth checking for conflicting fill requests? The code to do that might be kind of a pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think it's worth it because all of the current callbacks that specify this fill value set it to quiet_NaN. And yeah it was going to be a pain to make it general so I just didn't.

@wthrowe
Copy link
Member

wthrowe commented Oct 18, 2023

Looks good. Squash.

@knelli2 knelli2 force-pushed the multiple_callbacks branch from 25fe7b3 to 6034d16 Compare October 18, 2023 18:29
@kidder kidder merged commit 173f544 into sxs-collaboration:develop Oct 19, 2023
22 checks passed
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