Check Table in angular_separation functions

[nbiederbeck]

Hi there,

I spent too much time figuring out why my angular resolution was as bad as it was. I found out it was because of how astropy.coordinates.angular_separation behaves when given floats and quantities (i.e. one in deg, the other in rad).

angular_separation is what is used in our pyirf.utils.calculate_theta.

The problem was giving a Table instead of a QTable, where the columns are no quantities, thus the values get interpreted as floats and thus radians instead of degree (if they were in degree of course).

1. I added our check_table function to the functions that use angular_separation.
2. I checked for .unit being None, thus filtering out also the case where a Table ist passed (instead of a QTable). This fails with some "Column ... has incompatible unit None, expected ...". Here we could decide on checking isinstance(events, QTable).
3. I fixed the very repetitive wording of the wrong unit exception

The thing with these changes is, that incorrect usage before just yields wrong results, where the error (on the user side) is not obvious, after these changes incorrect usage fails.

[codecov]

Codecov Report

Base: 90.97% // Head: 91.45% // Increases project coverage by +0.47% 🎉

Coverage data is based on head (6ae3395) compared to base (61cadc2).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   90.97%   91.45%   +0.47%     
==========================================
  Files          40       40              
  Lines        1685     1720      +35     
==========================================
+ Hits         1533     1573      +40     
+ Misses        152      147       -5     

Impacted Files	Coverage Δ
pyirf/exceptions.py	100.00% <ø> (ø)
pyirf/tests/test_utils.py	100.00% <100.00%> (ø)
pyirf/utils.py	100.00% <100.00%> (+14.28%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[RuneDominik]

Regarding no. 2: Checking for table to be a QTable and raising a specific error might not be needed but should ease debugging if a user mistakenly passes a Table.

[kosack]

I would say if your function expects a QTable later on, you should just explicitly cast the input table to QTable in the function,

def my_func(table : Table):
    qtable = QTable(table)  # then use qtable later in the function   

That way you don't have to check for two different ways to access the columns, and then you also don't require QTables to be used as input (which lose access to some column metadata)

Edit: the metadata loss seems to be fixed now in astropy, so casting back and forth between Table and QTable now results in the same metadata (as long as you cast back to Table before writing). So casting to what you want to use seems to be the best solution and you do not lose anything in the process.