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

Array bounds/contains check? #1247

Open
mcclure opened this issue Dec 18, 2022 · 6 comments
Open

Array bounds/contains check? #1247

mcclure opened this issue Dec 18, 2022 · 6 comments
Labels
enhancement good first issue A good issue to start contributing to ndarray!

Comments

@mcclure
Copy link

mcclure commented Dec 18, 2022

I find myself very frequently having a coordinate and an ndarray Array2/Array3. I want to check whether the coordinate is within the bounds of the Array2/Array3 (IE, gte than 0,0,0, less than the array size). There doesn't appear to be a way to do this. There is "get" to return a value or None but that is not the same thing. Sometimes I want to know if the value is inbounds without, at that time, accessing the value.

It would be helpful to add a within(), contains() etc function to ArrayBase (I think adding it to Dim would also serve equivalent purposes).

(If this already exists and I just missed it, sorry)

@nilgoyette nilgoyette added enhancement good first issue A good issue to start contributing to ndarray! labels May 27, 2023
@nilgoyette
Copy link
Collaborator

IE, gte than 0,0,0, less than the array size

You're working with isize? I ask because, if I had to code it, I would probably use usize. All other [signed] types seem unfit.

@mcclure
Copy link
Author

mcclure commented May 29, 2023

You're working with isize? I ask because, if I had to code it, I would probably use usize. All other [signed] types seem unfit.

Hm, good question.

If it is okay to think out loud for a minute:

A thing I often do is use ndarray with glam::IVec2. Then when I need to load from ndarray at a particular coordinate I call a function like pub fn ivec_to_index(v:IVec2) -> (usize, usize) { (v.y as usize, v.x as usize) } to convert to a coordinate. Currently I do the bounds checking on the ivecs*, before converting to an ndarray index. In this context, a thing that would be very normal to do is to have an existing coordinate (such as (0, 5)) and add a "step" offset to it (such as (-1, 1)) and then test whether my new coordinate is within the bounds (and if not do something like abort, or wrap to the other side of the array maybe). So this seems to indicate maybe the right thing to do is keep my "within bounds?" logic within my project.

However, I think I can reproduce this situation with pure NDarray. Imagine if I had a walls:Array2<bool> representing something like a map in a video game, if the bool is true there is a wall. I want to put walls on all of the "edge" cells (so the player can't walk out of the map). Imagine if ArrayBase had a

pub fn contains<I>(&self, index: I) -> bool where I:NDIndex<D>;
pub fn contains_signed<I>(&self, index: I) -> bool where I:NDIndexButSignedSomehow<D>;

And I could write:

let mut walls:Array2<bool> = Default::default(); // Obviously it would be better to give it a size
for (y,col) in walls.axis_iter_mut(Axis(0)).enumerate() {
	for (x,mut cell) in col.iter_mut().enumerate() {
		'dir: for dir in 0..4 {
			let (mut ix, mut iy) = (x as isize, y as isize);
			match dir { // Look right, down, left, up
				0 => ix += 1, 1 => iy += 1,
				2 => ix -= 1, 3 => iy -= 1
			};
			if !walls.within_signed((iy, ix)) {
				*cell = true;
				break 'dir;
			}
		}
	}
}

Could I have written this code as only usizes, without using the isize indices? Not really, because I can't freely do math on those (for example I can't subtract 1 from the x axis without risking underflow). I'd have to add tests before subtracting, or something… it wouldn't be as convenient, in this case.

So, I think that contains() with usize indices would make sense and be useful, because in a situation where you already have usize indexes (which you usually do) but you are for whatever reason not sure if they're in-bounds for all axes, you could call it (and it would be more apt than .get(x).is_none()). This is probably the "usual case".

And a variant with isize-version indices, which is hardwired to just always return "false" if it finds less-than-zero components, would be situationally convenient, if someone happens to be on their own representing coordinates as isizes so they can do math on them. But I don't think this would be necessary, because the user could easily replicate it themselves by testing coordinate >= 0 && contains(coordinate as usize), or by just setting up the problem in the first place such that they only work with usizes. Does that make sense?


* Probably more information than you need: The way I normally do this is

fn within (at:IVec2, size:IVec2) -> bool {
	IVec2::ZERO.cmple(at).all() && size.cmpgt(at).all()
}

But this requires me to keep, for each array2, a separate ivec containing the array2's size… with a hypothetical contains(), I would not need to keep the "size" vector around and could do:

fn within<X> (at:IVec2, array:Array2<X>) -> bool {
	IVec2::ZERO.cmple(at).all() && array.contains(ivec_to_index(at))
}

@nilgoyette
Copy link
Collaborator

nilgoyette commented May 29, 2023

Thank you for adding more details.

Now that I think about it, we have a similar problem in medical imaging where we need to take a NxNxN patch around all voxels and apply some functions, or anything that works near the borders.

@adamreichold
Copy link
Collaborator

This is not a counter argument to having a contains method, but I just wanted to add that in similar situations, we have avoided the bounds checking in inner loops by padding out the underlying arrays. It does mean you have to skip those parts during the iteration, but it paid off to pay this constant cost compared checking bounds.

And of course, it does depend on whether the domain allows for sensible padding value. But for example, in a simulation where the array represented a map of cells, some of which were unreachable, it did change things at all to add some padding of unreachable cells outside which would never change the results.

@dacid44
Copy link

dacid44 commented Feb 13, 2024

Is this still something that's wanted? It's still marked as "good first issue", and I'm looking for ways to contribute, so I could probably implement this. Also, it would seem like a natural extension to an API like this would be for it to handle checking if an index is inside a slice (regardless of whether it's inside the full array). Should I work on that as well?

@nilgoyette
Copy link
Collaborator

nilgoyette commented Feb 20, 2024

@dacid44 Imo, yes, it would be used by some people, including me at my job.

to handle checking if an index is inside a slice

I'm not sure I understand. Can you please rephrase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue A good issue to start contributing to ndarray!
Projects
None yet
Development

No branches or pull requests

4 participants