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

Add Grid slicing #166

Open
cschwan opened this issue Aug 18, 2022 · 3 comments
Open

Add Grid slicing #166

cschwan opened this issue Aug 18, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@cschwan
Copy link
Contributor

cschwan commented Aug 18, 2022

Sometimes we'd like to work with a slice of a Grid, which means a subset of the contained orders, luminosities and/or bins.

We can already do this using the {order,bin,lumi}_mask parameters of Grid::convolute, but we have to remember these parameters when calling other methods of Grid. Instead it would be useful to add a new method Grid::slice, passing it the desired orders, bins and lumis, which then returns a GridSlice object containing the reference of a Grid object and the corresponding masks. This would allow

  • to have a simpler Grid::convolute function, because the mask parameters are already in the GridSlice object
  • the members GridSlice::{orders,bins,lumis} to return the correct subset of the original set of the Grid.

Furthermore, Grid itself is a GridSlice, so GridSlice would be a trait and Grid::slice would return a struct implementing the trait.

@cschwan cschwan added the enhancement New feature or request label Aug 18, 2022
@cschwan cschwan added this to the v0.6.0 milestone Aug 18, 2022
@cschwan cschwan self-assigned this Aug 18, 2022
@alecandido
Copy link
Member

I foresee ownership issues...

If you want to avoid data duplication, there should be a single owner, and multiple references around (one for each slice).
But then it would be complicated to have a mutable slice.

A possible solution might be to make the grid owning data through a reference counted Rc, and then make the slices own a Weak reference, to upgrade only when you call some slice method.

This is a solution that came to my mind, but I just wanted to point out the problem.

@cschwan
Copy link
Contributor Author

cschwan commented Aug 27, 2022

This won't be a problem, we'll have to model that similar to ndarray's ArrayBase::slice which uses explicit lifetime parameters. The Python interface will be more difficult, but in the first instance we can leave the API as is.

@alecandido
Copy link
Member

alecandido commented Aug 27, 2022

Ok, I learnt myself that explicit lifetimes are difficult (easy to fall down a path with a dead-end). But the moment there is a working example, following it is definitely a sensible choice.

I'll leave it up to you, and the moment we'll have the Rust implementation, we'll think about how to wrap it in Python. But I'm confident we'll find a suitable way to do it :)

@cschwan cschwan modified the milestones: v0.6.0, v1.0 Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants