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

Remove PrimitivesTag and PredicatesTag #1172

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Oct 5, 2024

  • Remove PrimitivesTag and PredicatesTag
  • Update check_valid_access_traits and provide CheckReturnTypeTag for predicates
  • Fix all examples, tests and benchmarks
  • Beef up access traits testing

Newer version of #997. Don't know if we want to rename AccessTraits to RangeTraits.

@aprokop aprokop added the refactoring Code reorganization label Oct 5, 2024
@aprokop aprokop force-pushed the remove_check_get_return_type branch from e1d10c2 to 4e21e0e Compare October 5, 2024 10:09
@aprokop aprokop changed the title Remove conditional checking of AccessTraits::get return type Remove PrimitivesTag and PredicatesTag Oct 5, 2024
@aprokop aprokop force-pushed the remove_check_get_return_type branch from 4e21e0e to 7d044fb Compare October 5, 2024 10:50
@aprokop aprokop force-pushed the remove_check_get_return_type branch from 7d044fb to 23603ba Compare October 27, 2024 13:58
@aprokop
Copy link
Contributor Author

aprokop commented Oct 27, 2024

Rebased to resolve conflicts.

@aprokop aprokop requested a review from dalg24 November 11, 2024 15:54
@aprokop
Copy link
Contributor Author

aprokop commented Nov 11, 2024

Rebased to resolve conflicts.

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Would you remind me whether we considered introducing a newer traits (not just a rename) and have the old access traits automatically translated to it if no specialization is provided?

@@ -58,27 +53,18 @@ struct ArborX::AccessTraits<ArborX::Experimental::AttachIndices<Values, Index>,
}
KOKKOS_FUNCTION static auto get(Self const &self, int i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KOKKOS_FUNCTION static auto get(Self const &self, int i)
KOKKOS_FUNCTION static auto get(Self const &self, Index i)

And Index(i) -> I below in both branches of the if constexpr

Copy link
Contributor Author

@aprokop aprokop Nov 12, 2024

Choose a reason for hiding this comment

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

This breaks custom index test in tstCompileOnlyAccessTraits.cpp, as we can't call Access::get(values, CustomIndex).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would potentially want us to revisit but it does not need to be done right now

@aprokop
Copy link
Contributor Author

aprokop commented Nov 11, 2024

Would you remind me whether we considered introducing a newer traits (not just a rename) and have the old access traits automatically translated to it if no specialization is provided?

I don't think so.

@dalg24
Copy link
Contributor

dalg24 commented Nov 11, 2024

Would you remind me whether we considered introducing a newer traits (not just a rename) and have the old access traits automatically translated to it if no specialization is provided?

I don't think so.

What are your thoughts about it? I expect that is implementable and may be nice to have but I lost track of all the changes and maybe that's just overkill.

@aprokop
Copy link
Contributor Author

aprokop commented Nov 12, 2024

What are your thoughts about it? I expect that is implementable and may be nice to have but I lost track of all the changes and maybe that's just overkill.

Just so that I understand, you are talking about introducing something like RangeTraits that would not have tags, using it internally, and providing specialization that would take tagged AccessTraits? I don't quite understand what you mean by "have the old access traits automatically translated to it if no specialization is provided".

@dalg24
Copy link
Contributor

dalg24 commented Nov 12, 2024

Just so that I understand, you are talking about introducing something like RangeTraits that would not have tags, using it internally, and providing specialization that would take tagged AccessTraits?

Yes pretty much

I don't quite understand what you mean by "have the old access traits automatically translated to it if no specialization is provided".

What you said above, "providing specialization that would take tagged AccessTraits"

@aprokop
Copy link
Contributor Author

aprokop commented Nov 12, 2024

Yes pretty much

I can try to implement this. So, we would advertise new traits (RangeTraits?) but users won't have to rewrite the existing ones? Wonder how much benefit there is, as we already broke backwards compatibility in the indexes constructors. We would need to do a bunch of renaming, too, from AccessValues to RangeValues and similar.

@dalg24
Copy link
Contributor

dalg24 commented Nov 12, 2024

I can try to implement this. So, we would advertise new traits (RangeTraits?) but users won't have to rewrite the existing ones?

Correct

Wonder how much benefit there is, as we already broke backwards compatibility in the indexes constructors.

That was my question

@aprokop
Copy link
Contributor Author

aprokop commented Nov 12, 2024

Wonder how much benefit there is, as we already broke backwards compatibility in the indexes constructors.

That was my question

I see little. I'd rather have users to go through upgrade once, with the major version release. In addition, I think it would give users a chance to reexamine their AccessTraits structures to see what they actually need to return now that they are not limited to points and boxes.

I do want to decide what we should call the traits. Doesn't have to be done here, though.

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Fine with renaming later

@aprokop aprokop added the backward incompatible Modifications that may break users' code label Nov 12, 2024
@aprokop aprokop merged commit 7c4ff52 into arborx:master Nov 12, 2024
2 checks passed
@aprokop aprokop deleted the remove_check_get_return_type branch November 12, 2024 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward incompatible Modifications that may break users' code refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants