Skip to content

Conversation

robertmaynard
Copy link
Collaborator

@robertmaynard robertmaynard commented Feb 25, 2022

Changes in the work include:

  • Internally use linear_space for iterating
  • Simplify type and value iteration in state_iterator::build_axis_configs
  • Store the iteration space in axes_metadata
  • Expose zip and user spaces to user
  • Add tests for linear, zip, and user
  • Add examples for zip and user

Fixes #68

Changes in the work include:
- [x] Internally use linear_space for iterating
- [x] Simplify type and value iteration in `state_iterator::build_axis_configs`
- [x] Store the iteration space in `axes_metadata`
- [x] Expose `tie` and `user` spaces to user
- [x] Add tests for `linear`, `tie`, and `user`
- [x] Add examples for `tie` and `user`
Copy link
Collaborator

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

re: naming, I'd stick with zip as opposed to tie.

I think "zip" is more intuitive and descriptive based on the precedence of things like thrust::zip_iterator and std::ranges::zip_view.

Comment on lines 53 to 56
virtual std::unique_ptr<axis_space_base> do_clone() const = 0;
virtual detail::axis_space_iterator do_iter(axes_info info) const = 0;
virtual std::size_t do_size(const axes_info &info) const = 0;
virtual std::size_t do_valid_count(const axes_info &info) const = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice to get some docs on the concept of a "axis space" as well as the semantics of these functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, same for the linear_axis_space and other subclasses below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added more documentation

@robertmaynard robertmaynard force-pushed the fea/axes_iteration_space branch from 182f1da to a25f578 Compare February 28, 2022 19:23
@robertmaynard robertmaynard force-pushed the fea/axes_iteration_space branch from 0a2130f to c3c86e1 Compare February 28, 2022 21:44
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

Did an initial round of review, but didn't finish -- sharing my comments so far. Will give this a closer look later.

In the meantime, can you go through this and add documentation for the new interfaces?

Having a high-level overview of how these pieces fit together would also be helpful, maybe include that in the class docs for nvbench::detail::axes_iterator.

@robertmaynard robertmaynard force-pushed the fea/axes_iteration_space branch 3 times, most recently from 5dc6c83 to d8d80e4 Compare April 12, 2022 16:22
@robertmaynard robertmaynard force-pushed the fea/axes_iteration_space branch from d8d80e4 to ba8356f Compare April 12, 2022 16:35
@robertmaynard robertmaynard force-pushed the fea/axes_iteration_space branch from 282754b to 5708e6c Compare August 31, 2022 15:08
@bdice
Copy link
Contributor

bdice commented Aug 7, 2024

Can we revive this PR? Are there blockers?

Copy link

copy-pr-bot bot commented May 1, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@alliepiper
Copy link
Collaborator

/ok to test 4defa02

@alliepiper
Copy link
Collaborator

/ok to test 4bd5690

@alliepiper alliepiper force-pushed the fea/axes_iteration_space branch from c820ec0 to a2bf266 Compare May 2, 2025 18:53
@alliepiper
Copy link
Collaborator

/ok to test

Copy link

copy-pr-bot bot commented May 2, 2025

/ok to test

@alliepiper, there was an error processing your request: E1

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/

@alliepiper
Copy link
Collaborator

/ok to test a2bf266

@alliepiper
Copy link
Collaborator

/ok to test c8909c7

@alliepiper alliepiper marked this pull request as draft May 7, 2025 20:43
@alliepiper
Copy link
Collaborator

I see a way to simplify this. Marking as draft as I work on the implementation.

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.

Add a zip_axis / tuple_axis that iterates through multiple parameter axes in lockstep
4 participants