Skip to content

Conversation

@yfukai
Copy link
Contributor

@yfukai yfukai commented Oct 15, 2025

This pull request introduces support for arbitrary strides in the GraphArrayView class, allowing for efficient downsampling and subsetting of graph-based array data. I'm planning to benchmark this approach and mark this ready for review if I find this approach efficient.

@JoOkuma
Copy link
Member

JoOkuma commented Oct 15, 2025

That will be awesome, @yfukai.
Feel free to add the benchmark to the benchmark folder.

It's very rudimentary, and we don't automatically check in the CI if the performance gets worse, but it's still a way to keep track of it.

Comment on lines 128 to 131
def mask_indices(
self,
offset: NDArray[np.integer] | int = 0,
) -> tuple[NDArray[np.integer], ...]:
Copy link
Member

@JoOkuma JoOkuma Oct 15, 2025

Choose a reason for hiding this comment

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

@yfukai, I don't know if it's faster or slower, but would it make sense to use this

        def mask_indices(
                self,
                offset: NDArray[np.integer] | int = 0,
                strides: NDArray[np.integer] | int = 1,  # new parameter
        ) -> tuple[NDArray[np.integer], ...]:

With the previous implementation?
b8617fc

I remember you updated this because the binary mask approach was much faster.

Copy link
Member

Choose a reason for hiding this comment

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

and use this only if strides != 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion! Maybe we should benchmark different approaches.

@yfukai
Copy link
Contributor Author

yfukai commented Oct 16, 2025

Hi thanks @JoOkuma! Would it be fine if I rewrite or add benchmarks using asv?

@JoOkuma
Copy link
Member

JoOkuma commented Oct 16, 2025

Hi thanks @JoOkuma! Would it be fine if I rewrite or add benchmarks using asv?

@yfukai, that would be awesome! Can you do it in a separate PR?

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