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

Tpetra: FixedHashTable says it has one pair when default initialized - expecting zero #13034

Closed
romintomasetti opened this issue May 22, 2024 · 2 comments

Comments

@romintomasetti
Copy link
Contributor

Description

@tpetra

When default-initializing Tpetra::Details::FixedHashTable, the numPairs method returns 1. I think it should return 0.

Here is a minimal example:

TEST(FixedHashTable, num_pairs_when_default_initialized)
{
    using fixed_hash_table_t = Tpetra::Details::FixedHashTable<int, int, Kokkos::Serial>;

    fixed_hash_table_t table {};

    ASSERT_EQ(table.numPairs(), 1); // I would expect 0 here
}

The culprit is:

KOKKOS_INLINE_FUNCTION offset_type numPairs () const {
// NOTE (mfh 26 May 2015) Using val_.extent(0) only works
// because the table stores pairs with duplicate keys separately.
// If the table didn't do that, we would have to keep a separate
// numPairs_ field (remembering the size of the input array of
// keys).
if (this->hasContiguousValues ()) {
return val_.extent (0) + static_cast<offset_type> (lastContigKey_ - firstContigKey_);

because when default initialized, FixedHashTable::hasContiguousValues() returns true, lastContigKey_ = ::Kokkos::ArithTraits<int>::min() and firstContigKey_ = ::Kokkos::ArithTraits<int>::max().

Note that I can workaround the problem by constructing the table with a default-initialized view of keys:

fixed_hash_table_t table( typename fixed_hash_table_t::keys_type {} );
@csiefer2
Copy link
Member

That looks... not right. Will look at a fix for that once the AT has cleared out the current mess.

@csiefer2
Copy link
Member

PR posted. @romintomasetti That look good?

csiefer2 added a commit that referenced this issue Jun 7, 2024
* Update Tpetra_Details_FixedHashTable_decl.hpp

* Tpetra: Adding associated test

* Update packages/tpetra/core/test/HashTable/FixedHashTableTest.cpp

Co-authored-by: Tomasetti Romin <[email protected]>

* Update packages/tpetra/core/src/Tpetra_Details_FixedHashTable_decl.hpp

Co-authored-by: Tomasetti Romin <[email protected]>

* Update Tpetra_Details_FixedHashTable_decl.hpp

* Update packages/tpetra/core/src/Tpetra_Details_FixedHashTable_decl.hpp

Co-authored-by: Tomasetti Romin <[email protected]>

* Update FixedHashTableTest.cpp

---------

Co-authored-by: Tomasetti Romin <[email protected]>
@csiefer2 csiefer2 closed this as completed Jun 7, 2024
@jhux2 jhux2 added this to Tpetra Aug 12, 2024
@jhux2 jhux2 moved this to Done in Tpetra Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

2 participants