Position sampling in Environments with holes.

[gsivori]

No description provided.

[TomGeorge1234]

Thanks for the PR!! I left a couple of comments. Can you provide a description of what bug this fixes/functionality this provides, I'm a bit lost as to the purpose of the PR

[gsivori]

Hi ! Thanks. I could not find your comments but let me describe the purpose of the PR (I think in the process of submitting the PR, VisualCode Studio did not attach my description so it is empty somehow..)/
The PR includes the function polygon_area(hole) in utils.py -- it is used to subtract from the total area available to be used for sampling legal positions in the environment. Originally, the function sample_positions in Environment.py, when using the uniform or uniform_jitter methods, would try to sample from the environment as many as it can but if it there are remaining positions requested: it will fall back to sample randomly until all positions are within the environment. The problem with this approach is that if you include holes in the environment and would like to sample uniformly or uniformly jittered positions, they end up being quite random if you do not take into account the area taken up by holes. So I am requesting with this PR to allow two things:

1. to consider the area taken up by holes such that when sampling uniformly (jittered or not) the area considered is appropriate and,
2. that you can force it obey the method (e.g. uniform) by resampling same positions (e.g. Place Cells with the same sampled position) when you have remaining positions to sample (n_remaining > 0).
   I hope this is easy to understand.
   I can provide an example if its not clear. But you may try this yourself by sampling a high number of positions (e.g. n=500) in an environment with holes.

Thanks a lot !!

[TomGeorge1234]

Can we reference this param in the doc string

[TomGeorge1234]

Where does this formula come from? Also is it limited to holes with four points? By the way we already list shapely as a requirement so we could use shapely.Polygon.area for this...just a thought

[TomGeorge1234]

That's interesting I've never heard of this formula before. Could you (a) either refer to it as the shoelace formula in the doc string or (b) use shapely.geometry.Polygon(hole).area instead.

Also, clarify that isn't just 4-corner holes that works, its any sized holes. Finally, in the docstrng make sure you remember to clarify the intended shape of holes ((N, 2) for N >= 3 presumably). Thanks!

[gsivori]

Ah, excellent! I did not realize shapely was imported. This is the same formula that shapely uses: the formula to calculate the area of a polygon given the list of points considering connectedness across the list of points. So we do not this in utils.py.

[TomGeorge1234]

Ok, nice! I took a better look. So basically, if there are holes in the Environment its area is accountably smaller so the "delta" (distance between uniform samples) is smaller too. This then uniformly scatters points but presumably lots of them fall in the holes which, when removed, brings the numbers to roughly what we want.

I would recommend:

• A check that there aren't too many samples in the environment after the ones in the holes have been removed.
• To avoid over complicating the code lets not have the force_sample flag. My feeling is that now you have correctly accounted for the area there should only be a small-ish number of correction to make. It's ok just to make these random since there won't be many. Conversely, your current method just re-samples them from the already approved holes and would create duplicates which isn't ideal.

Did I understand correctly, and do you agree?

[gsivori]

Yes, you're correct. This PR basically accounts for the holes or illegal spaces in the environment and adjusts "delta" accordingly so that uniform samples fall into a more fine-grained sample-able space. If this is not accounted for, lots of sampled positions are then randomly sampled and the final location of these positions looks entirely random.

I think you may be misguided by the amount of samples you typically use in your examples. In the simulations that I wish to run, I have many Poisson units that have receptive fields (e.g. 1000) and this number results in a lot of random positions scattered across the environment with little structure . But imagine these are, say, grid cells: such grid structure then is almost impossible to achieve if the sample_positions returns random positions. This is why I argue in favor of a force_method flag to allow for repeated positions (e.g. cells that have similar receptive field in terms of the receptive field center).

Thus, regarding your recommended changes, I feel like it should be OK to allow sampling from the same because duplicates, while not ideal, do not necessarily have the same width or shape; these would only be regarding the center of a given receptive field. On the other hand, force_method = false would fall back to allow random sampling.

Let me know your thoughts! I will update the PR with the changes considering the previous comments.

[TomGeorge1234]

I see your point about the scale of the issue once N gets large but my issue is that I don't think many people will (ever) want sampled positions to have repeats in them. So we're adding a whole extra param potentially just for this one use case.

Maybe instead, illegal positions could be resampled from the existing ones (like you propose) plus some random jitter of size delta / 2. This would actually just be one extra line. I'd be happy to go for this instead of randomly resampling the illegal points to save the extra kwarg. Something like:

positions_remaining += np.random.uniform(-0.45 * delta, 0.45 * delta, positions.shape)

Sorry to push back on this, just trying to prevent feature creep.

[gsivori]

I like your approach. I think this is a better approach in fact because if one samples at uniform and exceeds the available sampleable positions, a jittered sample at delta/2 is better than at delta (as it will look as if method was uniform_jittered when the user wanted uniform).

Double check if the code makes sense to you, and let me know your thoughts!

[TomGeorge1234]

I took a look and these look good to me!

[TomGeorge1234]

thanks for the PR Gaston! really appreciate the effort :))