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

[Core] PointerVectorSet #11363

Open
sunethwarna opened this issue Jul 6, 2023 · 4 comments
Open

[Core] PointerVectorSet #11363

sunethwarna opened this issue Jul 6, 2023 · 4 comments

Comments

@sunethwarna
Copy link
Member

sunethwarna commented Jul 6, 2023

Description
Recently me and @matekelemen (and many other also have complained in past: refer #9227) that there are some inconsistencies in the PointerVectorSet and how it is supposed to behave. The followings are the points which we discovered:

  1. Not guranteed to be ordered or unique.
  2. Has a confusing interface with mixed responsibilities.

1: Not guranteed to be ordered or unique

The underlying storage of the PointerVectorSet can be mutated and can lose its Set properties in the following cases.

  1. PointerVectorSet::GetContainer - The container can be freely mutated without triggering an update (sort and unique)
  2. PointerVectorSet::push_back - Array is appended but not sorted or made unique.
  3. PointerVectorSet::find - Mutates the data structure if not sorted. (Problematic in OpenMP as well)

2: Has a confusing interface with mixed responsibilities

The PointerVectorSet has following methods which give the impression it does not gurantee the Set properties.

  1. PointerVectorSet::IsSorted, PointerVectorSet::Sort, PointerVectorSet::Unique - Is giving wrong impressions about the assumed set properties.
  2. Mixed responsibilities (separating them would allow using the features individually as well):
    a. ordered set data structure
    b. value-like access to an array of pointers

Necessary changes

  1. Remove PointerVectorSet::GetContainer (both mutable and immutable)
  2. Replace push_back with insert(begin, end)
  3. Remove sorting in the mutable find
  4. Remove pop_back
  5. Remove Sort, Unique, IsSorted
  6. Remove mSortedPartSize, mMaxBufferSize and their getters and setters.
  7. Change insert(position, value) => insert(hint, value)

This would require multiple PRs with incremental changes, specially when replacing push_back.

Suggestion

Instead of rolling our own implementation, we suggest using a 3rd party implementation, because this is a solved problem. One possible library is tessil ordered_set. It is ordered and contiguous in memory and is header-only (3 files).

FYI @philbucher @loumalouomega

@loumalouomega
Copy link
Member

loumalouomega commented Jul 6, 2023

Everything you said: YES

This is a class is used everywhere, but as you said requires a lot of refactor.

@loumalouomega
Copy link
Member

Everything you said: YES

This is a class is used everywhere, but as you said requires a lot of refactor.

With modern container, we can simplify most of the code

@matekelemen
Copy link
Contributor

Just to be clear, this isn't a quality-of-life issue. It is a potential source of bugs that are difficult to track down but result in soft errors. It's critical that we fix this, and doing so may help us fix bugs that we didn't know about or had no idea that they originated from here.

@roigcarlo
Copy link
Member

@KratosMultiphysics/technical-committee We agree that changes need to be made. In general we agree with the proposal and we intend to:

  • First extend the interface to accomodate the insert changes
  • Start removing the interface functions that cause the PointerVectorSet to break

Porposal for the interface will be:

    iterator insert(TContainerType new_data) 
    {
        iterator hint = calchint();
        this.insert(hint, new_data.begin(), new_data.end()); 
    }

    iterator insert(const TContainerType::Iterator pBegin, const TContainerType::Iterator pEnd)
    {
        iterator hint = calchint();
        this.insert(hint, pBegin, pEnd); 
    }

    iterator insert(Iterator hint, TContainerType new_data) 
    {
        this.insert(hint, new_data.begin(), new_data.end()); 
    }

    iterator insert(Iterator hint, const TContainerType::Iterator pBegin, const TContainerType::Iterator pEnd)
    {
        // Actual code
        mData.sort()
        mData.unique()
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🆕 New
Development

No branches or pull requests

8 participants