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

[ENH] Level capabilities refactor for hashed level #24

Closed
wants to merge 3 commits into from

Conversation

adam2392
Copy link
Contributor

@adam2392 adam2392 commented Jun 7, 2023

Towards: #19

The hashed.hpp file implemented its own iteration_helper class, whereas the Coiterate API relies on each level having iteration helper within a level_capabilities namespace.

This PR attempts to refactor the iteration_helper part of hashed into the coordinate_iterate.hpp file.

Bottlenecks

For some reason I get compilation errors that iter_helper is not found, even tho it is now part of the hashed level through the level_capabilities namespace:

/Users/adam2392/Documents/xsparse/test/source/hashed_test.cpp:31:34: error: no member named 'iter_helper' in 'xsparse::levels::hashed<std::tuple<>, unsigned long, unsigned long, xsparse::util::container_traits<std::vector, std::set, std::unordered_map>, xsparse::level_properties<false, false, false, false, false>>'
    for (auto const [i2, p2] : h.iter_helper(ZERO))
                               ~ ^

and also

/Users/adam2392/Documents/xsparse/include/xsparse/util/base_traits.hpp:34:9: error: static_assert failed due to requirement 'std::is_convertible_v<xsparse::util::container_traits<std::vector, std::set, std::unordered_map>, unsigned long>' "`PK` must be convertible to uintptr_t."
        static_assert(std::is_convertible_v<PK, uintptr_t>,
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Notes from July 11th, 2023:

  • Change Levels::LevelCapabilities::iteration_helper to Levels::iteration_helper to remove requirement of namespace LevelCapabilities in Coiteration
  • Add maybe-unused parameter I to the initialization of hashed level iteration_helper

@bharath2438
Copy link
Collaborator

Hey @adam2392 @hameerabbasi,
I was trying to find a better way of accessing LevelCapabilities of hashed and found that using LevelCapabilities = typename xsparse::levels::hashed<std::tuple<LowerLevels...>, IK, PK, ContainerTraits, _LevelProperties>; in hashed.hpp without having to move the iteration logic to coordinate_iterate.hpp does work.
Another reason why we should avoid moving the code is because https://github.com/hameerabbasi/xsparse/blob/9f47a2cfc37ac9a2bf7197f6489df87bb2bddb83/include/xsparse/levels/hashed.hpp#LL132C14-L132C14 here, we need m_crd which is in the scope of hashed level.

Fixing this and adding the [[maybe_unused]] I parameter should make hashed consistent with other levels and (hopefully) allow coiteration with other levels.

@adam2392
Copy link
Contributor Author

Hey @adam2392 @hameerabbasi, I was trying to find a better way of accessing LevelCapabilities of hashed and found that using LevelCapabilities = typename xsparse::levels::hashed<std::tuple<LowerLevels...>, IK, PK, ContainerTraits, _LevelProperties>; in hashed.hpp without having to move the iteration logic to coordinate_iterate.hpp does work. Another reason why we should avoid moving the code is because https://github.com/hameerabbasi/xsparse/blob/9f47a2cfc37ac9a2bf7197f6489df87bb2bddb83/include/xsparse/levels/hashed.hpp#LL132C14-L132C14 here, we need m_crd which is in the scope of hashed level.

Fixing this and adding the [[maybe_unused]] I parameter should make hashed consistent with other levels and (hopefully) allow coiteration with other levels.

Hi @bharath2438 I've opened up #25 to supersede this PR and added that logic. Lmk if that is what you had in mind?

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.

2 participants