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

Defining regions without specifying center or shape parameters #446

Open
larrybradley opened this issue Mar 28, 2022 · 3 comments
Open

Defining regions without specifying center or shape parameters #446

larrybradley opened this issue Mar 28, 2022 · 3 comments

Comments

@larrybradley
Copy link
Member

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 (#430 (comment)).

This topic came up before (by me 6 years ago) in #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?

@keflavich
Copy link
Contributor

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.

@adonath
Copy link
Member

adonath commented Mar 28, 2022

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
Copy link
Member

adonath commented Mar 28, 2022

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants