regions icon indicating copy to clipboard operation
regions copied to clipboard

Defining regions without specifying center or shape parameters

Open larrybradley opened this issue 3 years ago • 3 comments

In #430, I added validation to the shape parameters requiring radius, width, and height parameters to be strictly positive (> 0). That makes complete sense, but it broke @adonath 's workflow where he had been using np.nans as placeholders in some cases (https://github.com/astropy/regions/pull/430#issuecomment-1076349753).

This topic came up before (by me 6 years ago) in https://github.com/astropy/regions/issues/56, but it was never implemented. The workaround of course is to define the region with valid (non-nan, non-zero) dummy values, which can be changed later (the center and shape parameters are mutable). I think this needs discussion with other devs.

@keflavich, @astrofrog, @dhomeier -- any thoughts or ideas?

larrybradley avatar Mar 28 '22 16:03 larrybradley

Can we maybe make a separate RegionTemplate class/set of classes? Part of the reason we require a center, for example, is that that is needed (in combination w/WCS) to convert to pixel coordinates. A SkyRegion object with any parameters undefined would simply not be convertible to pixel coordinates, etc, so may of its attributes would be invalid, right? That leads me to think a different class with a different attribute set would be best.

keflavich avatar Mar 28 '22 17:03 keflavich

Just to give some context for our use-cases:

While the regions API was mostly designed for user interaction, it also worked great for us in production code. E.g. in Gammapy we provide convenience access to gamma-ray catalogs, where the measured source extension can be nicely represented as a Region for plotting it on sky images or quick aperture like measurements. However there are still often cases where part of the information might be missing and this is where we put np.nan as placeholder instead. For almost all following applications this just works out of the box:

  • Matplotlib will simply hide patches with np.nan values and does not raise when plotting the region
  • Numpy will just pass on the np.nan values through any computation, without raising
  • Astropy coordinate transforms just happily handle NaN values and pass them through, same for SkyCoord or Quantity
  • Very similar for most Scipy methods...

So using NaN values in the region API now raises, which requires special cases and try / except statements in various places. I think not allowing NaN values and raising an error instead is a significant deviation from usual Numpy / Scipy / Matplotlib behavior, where NaN values are typically just passed through computations (with warnings of course...).

adonath avatar Mar 28 '22 17:03 adonath

Just for completeness: my comment above is from the perspective of production code, where regions are defined algorithmically or programmatically. I agree that supporting NaN from an end-user perspective is much less of importance...

adonath avatar Mar 28 '22 18:03 adonath