Filter instance method unsafe? about euro-neurips-2022 HOT 9 CLOSED

Since it's explicitly intended to work with masks, I think a dtype coercion to bools as the top of the function is fine. E.g.

mask = mask.astype(bool)

from euro-neurips-2022.

mask = mask.astype(bool)

These functions are executed quite a few times, so it should ideally not consume any time.

Only issue with that is that it will still convert np.array([0,1,2]) into np.array([False, True, True]).
But I guess we should then trust ourselves to not try to put any values > 1 into the mask 😅

Or is it faster to just check is the mask is a boolean, and assume a coercion would be done outside this function?
Preferably in some (temporary?) assert that we could get rid of in the final solver submission?

from euro-neurips-2022.

These functions are executed quite a few times, so it should ideally not consume any time.

When I last ran the profiler for rollout, this function did not show up at all. I do not think there's a reason to worry about performance already - and when there is, the profiler will tell us and we can always decide on something else then.

But I guess we should then trust ourselves to not try to put any values > 1 into the mask 😅

That's how I'm leaning. An array of indices is not a mask. Only an array of booleans or the like is.

Typically a mask should match the shape of the thing that's being masked. Maybe we should check that?

from euro-neurips-2022.

When I last ran the profiler for rollout, this function did not show up at all. I do not think there's a reason to worry about performance already - and when there is, the profiler will tell us and we can always decide on something else then.

Well, every additional check will take some time. But it might not be much indeed.

Typically a mask should match the shape of the thing that's being masked. Maybe we should check that?

If the mask is of dtype boolean, numpy will give an error itself if the shapes does not match.

So my preference would be to only check if mask.dtype is np.dtype('bool').
This check should be fast, and independent of the array size.

Only downside of this is that you cannot put a list of bools as mask anymore, but I think we already didn't do that.

from euro-neurips-2022.

Well, every additional check will take some time. But it might not be much indeed.

We're talking maybe a few hundred nanoseconds here (I'm on my phone, cannot check, but the profiler's resolution is a millisecond and it didn't show up). Why program this in Python if we care about nanoseconds?

So my preference would be to only check if mask.dtype is np.dtype('bool').

Checking types is an anti-pattern that I'd like to avoid. If it works it works, and the outcome's basically on the caller to verify.

So maybe we should just leave this be. If the argument's unexpected we can leave that to the call site to deal with. The thing being named mask is pretty clear about what's expected already.

from euro-neurips-2022.

Okay.

Python doesn't really allow to include dtypes in the type hinting right? Something like mask: np.array[bool]

Then we would at least have some soft-check in pycharm, but otherwise agreed to leave it as is.

from euro-neurips-2022.

That'd work for lists in Py3.9 and up (but not Py3.8, which is the version used by the submission env). I'm not sure about numpy arrays. With

from __future__ import annotations 

we can always pretend it does, I think. But I'm not sure if most editors will understand.

I don't have access to an IDE right now. Can you tinker with this a bit, if you want?

from euro-neurips-2022.

I tried some different things. You can definitely add a type specification, all this is accepted (but the 2nd or 3th probably prefered) mask: np.ndarray[bool] , mask: Union[bool], mask: np.typing.ArrayLike[bool].

But couldn't get PyCharm to actually care if you tried to put in an array of int's instead of bools.

from euro-neurips-2022.

I think we just have to be careful. There's a lot more in the Python code that can go wrong. I myself made a mistake with putting ints in the mask, but such issues are easily identified by regression tests. I just run

python solver.py --strategy rollout --instance instances/ORTEC-VRPTW-ASYM-00c5356f-d1-n258-k12.txt --epoch_tlim 5

after every change and you'll find out quickly enough if things go right or wrong.

@jaspervd96 If you're also OK with just leaving it as is, could you then close this issue?

from euro-neurips-2022.