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

Introduce free functions for consensus algorithms. #13414

Merged
merged 2 commits into from May 9, 2022

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Feb 18, 2022

This addresses #13358. It introduces free functions for the consensus algorithms and uses them in all of the places where we currently use the run() functions that get their information through a number of std::function arguments.

I took the liberty to move the main parts of the documentation from the base class to the namespace everything is in. The new functions and the old classes now refer to this main documentation place.

/rebuild

@bangerth
Copy link
Member Author

Any thoughts?

Copy link
Member

@peterrum peterrum left a comment

Choose a reason for hiding this comment

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

I think it would be enough to have a single function Utilities::MPI::ConsensusAlgorithms::run(), which wraps the class Utilities::MPI::ConsensusAlgorithms::Selector (the function should simplify the usage; multiple options just make it complicated). But I guess the reason for having the functions nbx(), pex(), selector() is that you want to move in a follow-up PR the content of the classes there. At least this is how I interpret the comment:

introduce global functions that replace ConsensusAlgorithms::*::run().

As stated elsewhere I don't think it is a good idea to get rid of the classes.

@bangerth
Copy link
Member Author

I don't actually have much of an opinion on what to do with the classes. I had in mind to ask what people want to do with them.

There are currently only two uses of the Process class, both in mpi_compute_index_owner_internal.h. The last commit here (which currently causes a merge conflict, and which I'll drop again, but which you should continue to be able to see at 0812c30) removes one of these uses, and I would have eventually looked at the other use.

My main motivation for making these free functions instead of classes is so I can provide overloads. As mentioned in #13084, a substantial number of places where we currently use the consensus algorithm interface don't send an answer back for each request -- or, to be more clear, they send an empty vector. In other words, they really just do what Utilities::MPI::some_to_some() does (which currently uses something like the PEX algorithm). In all of these places, the code just chooses a more or less randomly chosen type for T2 (sometimes char, sometimes unsigned int), which isn't used. What I want to do is introduce overloads of the free functions for the case where no answer is required. Whereas the current interface is

      template <typename T1, typename T2>
      std::vector<unsigned int>
      selector(
        const std::vector<unsigned int> &targets,
        const std::function<std::vector<T1>(const unsigned int)>
          &create_request,
        const std::function<std::vector<T2>(const unsigned int,
                                            const std::vector<T1> &)>
          &answer_request,
        const std::function<void(const unsigned int, const std::vector<T2> &)>
          &             process_answer,
        const MPI_Comm &comm);

the interface where no answer is required would be

      template <typename T1>
      std::vector<unsigned int>
      selector(
        const std::vector<unsigned int> &targets,
        const std::function<std::vector<T1>(const unsigned int)>
          &create_request,
        const std::function<void(const unsigned int,
                                              const std::vector<T1> &)>
          &process_request,
        const MPI_Comm &comm);

One could have achieved the same by providing specializations of the existing classes, for example for the case T2=void (which would have neatly described that we don't want to send any answer). I just thought that overloading free functions provides a cleaner interface.

Either way, I'm happy to discuss future steps separately. I think this patch is a step forward by itself.

@bangerth
Copy link
Member Author

How should we proceed here?

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

I would be fine to have both classes (to encapsulate additional information where it is needed between the calls of different functions) and the free functions proposed here.

@peterrum
Copy link
Member

peterrum commented May 1, 2022

I would be fine to have both classes (to encapsulate additional information where it is needed between the calls of different functions) and the free functions proposed here.

I would like to keep these classes, since I see those as the place for future developments, like supporting multihop versions. I regard the free functions only as a way to improve the usability, which important - particularly for the user.

@bangerth
Copy link
Member Author

bangerth commented May 5, 2022

I'm ok with keeping the classes.

Are there objections to the patch as-is then?

// Implementation of the functions in this namespace.

template <typename T1, typename T2>
std::vector<unsigned int>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be inline?

Suggested change
std::vector<unsigned int>
inline std::vector<unsigned int>

Copy link
Member

Choose a reason for hiding this comment

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

You don't technically need the inline statement as these are templated functions and thus automatically inline (i.e., not causing duplicate symbols during linking if they get included from multiple compilations units). But we tend to write inline in most occurrences. (But I would not stop the PR from this minor style issue, it has waited for long already.)

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

Successfully merging this pull request may close these issues.

None yet

4 participants