search

gaioguys / GAIO.jl

A Julia package for set oriented computations.
MIT License
9 stars 4 forks source link

Boxes with Radius 0 #9

Closed MaHermann closed 4 years ago

MaHermann commented 4 years ago

Right now we allow a Box to have radius 0. This might be useful in some cases, but as is leads leads to silent unexpected behavior, so it should at least trigger some warning or fail.

Steps to reproduce:

using GAIO

partition = RegularPartition(Box(SVector(0.0,0.0),SVector(1.0,0.0)))
box_set1 = p[:]
box_set2 = p[p.domain.center]
length(intersect(box_set1,box_set2))

Expected Result: Length should be 1. Actual Result: Length is 0.

Reason: As the radius in the second dimension is 0, the entry in partition.scale is Inf. Base.unsafe_trunc(Int,Inf) gives the lowest negative number, and the check in the next line doesn't prevent this key from being added. Trying to access this box (e.g. via looping over box_set2 throws an error, but we probably should catch this much earlier.

Similar issues might also happen with negative radii, but these should probably just throw an error at creation (of either the Box or the Partition)

JosephaHilmer commented 4 years ago

Would it be reasonable for the cases, where we want the radius of the box to be zero, to 'secretly' change an entry of radius zero to a small epsilon >0?

MaHermann commented 4 years ago

Yes, I also favor not allowing it at all and have something like this in the cases where we want it. But then we probably shouldn't do this implicitly but rather state explicitly what's going on. So my prefered solution here is just throw an error if the radius is not >0 in some component.

JosephaHilmer commented 4 years ago

"rather state explicitly what's going on" - yes, you are right. Otherwise it could probably lead to missunderstandings from user-side...

cafaxo commented 4 years ago

From the call today: Disallow the construction of partitions with empty boxes (i.e. radius = 0 in a component). Disallow the construction of boxes that have a radius with negative component. I'll work on a PR with these changes.