[Core] Adding hash for `std::pair` #11312

loumalouomega · 2023-06-23T11:27:44Z

📝 Description

This PR adds missing hash definitions and tests for STL pair (std::pair). The changes include:

In the file "key_hash.h", a hash definition for pairs is added to the std namespace. It calculates the hash value of a given pair of values by combining the hash values of the individual elements using HasCombine.
In the file "test_key_hash.cpp" a new test case "HashSTLPair" is added to test the hash function for STL pairs.

🆕 Changelog

Adding hash for std::pair

philbucher · 2023-06-25T11:58:56Z

I thought that adding stuff to the std namespace is not allowed/bad practice?

Surely there must be a better way no?

@matekelemen do you have an idea?

loumalouomega · 2023-06-25T12:19:24Z

We add things to the std namespace in the stl_io.h file. El dom., 25 jun. 2023 13:59, Philipp Bucher ***@***.***> escribió:

…

I thought that adding stuff to the std namespace is not allowed/bad practice? Surely there must be a better way no? @matekelemen <https://github.com/matekelemen> do you have an idea? — Reply to this email directly, view it on GitHub <#11312 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEYQZAFO5NTMKTRZDI3C2ILXNARYZANCNFSM6AAAAAAZROC2DY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

matekelemen · 2023-06-25T13:16:19Z

I thought that adding stuff to the std namespace is not allowed/bad practice?

Surely there must be a better way no?

@matekelemen do you have an idea?

As far as I know, providing specializations for std::hash is the recommended method of hashing an object.

From cplusplus reference on std::hash: "Users can provide custom specializations for this template with these same properties."

From cppreference's page on std::hash: "User-provided specializations of hash also must meet those requirements.".

philbucher · 2023-06-25T13:20:40Z

We add things to the std namespace in the stl_io.h file.

Sounds like sth to be refactored then :)

EDIT: I couldnt find what you are referring to

loumalouomega · 2023-06-26T07:03:12Z

We add things to the std namespace in the stl_io.h file.

Sounds like sth to be refactored then :)

EDIT: I couldnt find what you are referring to

Sorry, I meant array_1d:

Kratos/kratos/containers/array_1d.h

Line 792 in 1866070

namespace std

philbucher · 2023-06-26T09:46:00Z

I see

I encourage to do this in accordance to what the standard proposes (which from what I can see does not requires namespace std stuff)

@matekelemen since you approved the other PR with the same thing, I leave this decision to you

matekelemen · 2023-06-26T11:01:21Z

I specifically requested this format from @loumalouomega, so I'm completely happy with it, apart from the unconstrained template parameters. However, now's the time to find a solution that everyone supports.

From my side, I'd go with specializing std::hash because users were meant to define specializations for their own types that they want hashed. Furthermore, we won't have to provide custom hashers for standard-conforming containers (std::unordered_set, std::unordered_map, etc.) because they use std::hash by default.

I don't see any issues with specializing std::hash, provided that

we're specializing for our own classes (and definitely not some object from a 3rd party lib)
the specialization is declared in the same header as the class

The reason why I'm hesitant in this case is because the template parameters are unconstrained, and this will poison std::pairs of non-Kratos types too. As it stands right now, I wouldn't merge it, but find some way of restricting the template parameters such that at least one of the two is a Kratos type.

@philbucher what do you think?

loumalouomega · 2023-06-26T13:22:43Z

The reason why I'm hesitant in this case is because the template parameters are unconstrained, and this will poison std::pairs of non-Kratos types too. As it stands right now, I wouldn't merge it, but find some way of restricting the template parameters such that at least one of the two is a Kratos type.

I think the implementation is general enough to avoid conflicts

matekelemen · 2023-06-26T13:51:08Z

I think the implementation is general enough to avoid conflicts

Generality is exactly the problem here. This will be super confusing if a 3rd party lib defines a specialization for std::pair with their own types. For example:

// some_3rd_party_header.hpp

#include <functional> // std::hash
#include <utility> // std::pair

namespace whatever {
struct S {};
} // namespace whatever

namespace std {
template <>
struct hash<pair<whatever::S,whatever::S>>
{
    std::size_t operator()(const pair<whatever::S,whatever::S>&) const noexcept {return 0;}
}; // struct hash
} // namespace std

// key_hash.h

/*...*/
namespace std {
template <class T1, class T2>
struct hash<pair<T1,T2>>
{
    std::size_t operator()(const pair<T1,T2>& rPair) const noexcept {return 1;}
}; // struct hash
} // namespace std
/*...*/

So what will this print?

#include <iostream>
#include "some_3rd_party_header.hpp"
#include "key_hash.h"

int main()
{
    std::pair<whatever::S,whatever::S> key;
    std::hash<std::pair<whatever::S,whatever::S>> hasher;
    std::cout << hasher(key) << std::endl;
}

How about this one? (include order is swapped)

#include <iostream>
#include "key_hash.h"
#include "some_3rd_party_header.hpp"

int main()
{
    std::pair<whatever::S,whatever::S> key;
    std::hash<std::pair<whatever::S,whatever::S>> hasher;
    std::cout << hasher(key) << std::endl;
}

=> Exactly, you had to look it up or run it on godbolt.

loumalouomega · 2023-06-26T14:00:37Z

So what do you suggest?

matekelemen · 2023-06-26T14:10:40Z

I have 2 things in mind, neither of which is too appealing:

find a way to check whether at least one of T1 or T2 comes from the Kratos namespace (I don't even know whether this can be done portably)
define separate specializations for each std::pair combo you want to use in std::hash (this is doable but can get painful if there are lots of types you want to support - just keep in mind that you can also do partial specializations to make your life easier)

matekelemen · 2023-06-26T14:12:01Z

I'm open to other ideas tho.

loumalouomega · 2023-06-26T14:15:57Z

I have 2 things in mind, neither of which is too appealing:

1. find a way to check whether at least one of `T1` or `T2` comes from the `Kratos` namespace (I don't even know whether this can be done portably)

2. define separate specializations for each `std::pair` combo you want to use in `std::hash` (this is doable but can get painful if there are lots of types you want to support - just keep in mind that you can also do partial specializations to make your life easier)

Probably better the second idea, despite being a pain in the ass. Besides, I removed the dependency of this in my current as I see is being probelmatic.

matekelemen · 2023-06-26T14:28:06Z

Also, depending on how much sense it makes for your particular case, you could just define a class that's semantically identical to std::pair, and specialize std::hash for that. For example:

namespace Kratos {
template <class TLocal, class TGlobal>
struct IndexPair
{
    TLocal local;
    TGlobal global;
}; // struct IndexPair
} // namespace Kratos

namespace std {
template <class TLocal, class TGlobal>
struct hash<Kratos::IndexPair<TLocal,TGlobal>>
{
    std::size_t operator()(const Kratos::IndexPair<TLocal,TGlobal>&) const noexcept {/*...*/}
}; // struct hash
} // namespace std

loumalouomega · 2023-06-26T14:52:44Z

Also, depending on how much sense it makes for your particular case, you could just define a class that's semantically identical to std::pair, and specialize std::hash for that. For example:

namespace Kratos {
template <class TLocal, class TGlobal>
struct IndexPair
{
    TLocal local;
    TGlobal global;
}; // struct IndexPair
} // namespace Kratos

namespace std {
template <class TLocal, class TGlobal>
struct hash<Kratos::IndexPair<TLocal,TGlobal>>
{
    std::size_t operator()(const Kratos::IndexPair<TLocal,TGlobal>&) const noexcept {/*...*/}
}; // struct hash
} // namespace std

For the moment I will keep this PR for discussion and revert the changes of this branch in my branch. It looks like it will be a long discussion

matekelemen · 2023-07-14T09:41:37Z

RiccardoRossi approved these changes

Did I miss a discussion on this?

philbucher · 2023-07-14T09:49:05Z

RiccardoRossi approved these changes

Did I miss a discussion on this?

@RiccardoRossi probably selected the wrong option 🤣

RiccardoRossi · 2023-07-14T10:45:48Z

sorry, i did not see the discussion.

RiccardoRossi · 2023-07-14T10:46:26Z

inserting in the std namespace is not allowed as you correctly pointed out (i did not see that ... sorry...)

this should be in the Kratos namespace

matekelemen · 2023-07-14T15:29:39Z

@RiccardoRossi the C++ standard library is specifically designed to allow user-defined specializations of std::hash, and it's the cleanest way to support our types in hash maps. I don't think there are any issues with extending the std namespace in this case.

What I am concerned about is that the template parameters here are unconstrained and can match any class, not just the ones Kratos defines.

loumalouomega · 2023-07-16T10:00:55Z

@RiccardoRossi the C++ standard library is specifically designed to allow user-defined specializations of std::hash, and it's the cleanest way to support our types in hash maps. I don't think there are any issues with extending the std namespace in this case.

What I am concerned about is that the template parameters here are unconstrained and can match any class, not just the ones Kratos defines.

A priori hash combine works with any class, so I wouldle it that way. It is just the combination of two existing hashes, nothing else.

RiccardoRossi · 2023-07-17T08:15:11Z

sorry, i did some more searching

https://en.cppreference.com/w/cpp/language/extending_std

concretely at that link it tells that "
Specializations of std::hash for program-defined types must satisfy Hash requirements.
"

implicitly telling that it can be specialized.

in short...i am out of my depth here, but it looks like Mate is right and it is allowed to specialize std::hash

this also tells that i should not be the one approving or not this, since obviously it escapes my knowledge

matekelemen · 2023-07-17T09:28:43Z

I don't think I should be the one making the decision here either; maybe @roigcarlo can help us out and put it up for discussion in the @KratosMultiphysics/technical-committee ?

I'll try summarizing my opinion:

Extending the std is fine in this case IF we can ensure that it won't interfere with third party libs we're using, or third party libs that might #include Kratos in the future. This effectively means that we must ensure that whatever we extend the std namespace with, will only affect classes coming from the Kratos namespace.

The problem I see is with the template declaration of the specialization:

template<typename T1, typename T2>
struct hash<std::pair<T1, T2>>

Since std::pair is obviously used outside Kratos as well, we'd have to make sure that T1 and/or T2can only match Kratos classes. However, this is not guaranteed in this case since T1 and T2are unconstrained and could match anything. This could interfere with 3rd party libraries we're using and result in unintended behaviour which would be extremely difficult to detect.

Right now, I have 3 solutions in mind but all of them are a bit ugly, so I'm open to other ideas:

Create a separate specialization for each exact pair we want hashing. For example:

template <class T1, class TValue, std::size_t ArraySize>
struct hash<pair<T1,Kratos::array_1d<TValue,ArraySize>>>

Define a pair in the Kratos namespace and define the generic specializations for that, instead of std::pair:

namespace Kratos {
template <class T1, class T2>
struct Pair {T1 first; T2 second;};
} // namespace Kratos

namespace std {
template <class T1, class T2>
struct hash<Kratos::Pair<T1,T2>>
...
}

Implement some template magic that detects whether the template parameter is from the Kratos namespace. Here's a working solution that exploits argument dependent lookup.

loumalouomega · 2023-07-17T09:50:29Z

But @matekelemen the hash for std::pair is not defined at all, not even std::pair<int, int>, and int is not a Kratos class

roigcarlo · 2023-07-17T11:58:09Z

My 2 cents: I tend to agree with @matekelemen arguments, except that for me the ideal solution would be

4 - Create our own hash class and use it explicitly where needed:

class pair_hash .... // The same code you put but is our hash and not the std one

....

int std::map<std::pair<TA,TB>,int,pair_hash> my_map;

Deriving from hash will only allow the user to remove the explicit hash_function from the type of the map/class where he wants t use it, but will make a mess with external libraries and pretty much everyone depending on a standard definition of the hash.

Note that, even if we go for that solution we can just define the type of our class like we do with other Kratos types and it keeps being transparent for the user.

Edit: If you like it better:

class pair_hash {
    public:
        template<class T1, class T2>
        operator() (const std::pair<T1,T2>) {...}
}

template<class TIdx1, class TIdx2, class TData>
using pair_map = std::map<std::pair<TIdx1, TIdx2>, TData, pair_hash>;

....

int Kratos::pair_map<int, string, int> my_map;

loumalouomega · 2023-08-21T11:58:20Z

Do I merge this?

roigcarlo · 2023-08-25T08:22:16Z

I still think that extending the std::pair is wrong and we should provide an external hash, but as you like.

loumalouomega · 2023-08-25T08:40:25Z

I still think that extending the std::pair is wrong and we should provide an external hash, but as you like.

Okay

loumalouomega added 2 commits June 23, 2023 13:22

[Core] Adding hash for std::pair

3246bb0

Update

868359d

loumalouomega added Enhancement Kratos Core labels Jun 23, 2023

loumalouomega requested review from roigcarlo and matekelemen June 23, 2023 11:27

loumalouomega requested a review from a team as a code owner June 23, 2023 11:27

Windows proof test

d9de576

loumalouomega added this to To do in LEGACY Technical Committee via automation Jun 26, 2023

RiccardoRossi approved these changes Jul 14, 2023

View reviewed changes

loumalouomega merged commit e5ac9ab into master Aug 25, 2023
11 checks passed

LEGACY Technical Committee automation moved this from To do to Done Aug 25, 2023

loumalouomega deleted the core/hash-std-pair branch August 25, 2023 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Adding hash for `std::pair` #11312

[Core] Adding hash for `std::pair` #11312

loumalouomega commented Jun 23, 2023

philbucher commented Jun 25, 2023

loumalouomega commented Jun 25, 2023 via email

matekelemen commented Jun 25, 2023 •

edited

philbucher commented Jun 25, 2023 •

edited

loumalouomega commented Jun 26, 2023

philbucher commented Jun 26, 2023

matekelemen commented Jun 26, 2023 •

edited

loumalouomega commented Jun 26, 2023

matekelemen commented Jun 26, 2023 •

edited

loumalouomega commented Jun 26, 2023

matekelemen commented Jun 26, 2023

matekelemen commented Jun 26, 2023

loumalouomega commented Jun 26, 2023

matekelemen commented Jun 26, 2023

loumalouomega commented Jun 26, 2023

matekelemen commented Jul 14, 2023

philbucher commented Jul 14, 2023

RiccardoRossi commented Jul 14, 2023

RiccardoRossi commented Jul 14, 2023

matekelemen commented Jul 14, 2023

loumalouomega commented Jul 16, 2023

RiccardoRossi commented Jul 17, 2023 •

edited

matekelemen commented Jul 17, 2023

loumalouomega commented Jul 17, 2023

roigcarlo commented Jul 17, 2023 •

edited

loumalouomega commented Aug 21, 2023

roigcarlo commented Aug 25, 2023

loumalouomega commented Aug 25, 2023

[Core] Adding hash for std::pair #11312

[Core] Adding hash for std::pair #11312

Conversation

loumalouomega commented Jun 23, 2023

philbucher commented Jun 25, 2023

loumalouomega commented Jun 25, 2023 via email

matekelemen commented Jun 25, 2023 • edited

philbucher commented Jun 25, 2023 • edited

loumalouomega commented Jun 26, 2023

philbucher commented Jun 26, 2023

matekelemen commented Jun 26, 2023 • edited

loumalouomega commented Jun 26, 2023

matekelemen commented Jun 26, 2023 • edited

loumalouomega commented Jun 26, 2023

matekelemen commented Jun 26, 2023

matekelemen commented Jun 26, 2023

loumalouomega commented Jun 26, 2023

matekelemen commented Jun 26, 2023

loumalouomega commented Jun 26, 2023

matekelemen commented Jul 14, 2023

philbucher commented Jul 14, 2023

RiccardoRossi commented Jul 14, 2023

RiccardoRossi commented Jul 14, 2023

matekelemen commented Jul 14, 2023

loumalouomega commented Jul 16, 2023

RiccardoRossi commented Jul 17, 2023 • edited

matekelemen commented Jul 17, 2023

loumalouomega commented Jul 17, 2023

roigcarlo commented Jul 17, 2023 • edited

loumalouomega commented Aug 21, 2023

roigcarlo commented Aug 25, 2023

loumalouomega commented Aug 25, 2023

[Core] Adding hash for `std::pair` #11312

[Core] Adding hash for `std::pair` #11312

matekelemen commented Jun 25, 2023 •

edited

philbucher commented Jun 25, 2023 •

edited

matekelemen commented Jun 26, 2023 •

edited

matekelemen commented Jun 26, 2023 •

edited

RiccardoRossi commented Jul 17, 2023 •

edited

roigcarlo commented Jul 17, 2023 •

edited