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

Check Table in angular_separation functions #191

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

nbiederbeck
Copy link
Contributor

@nbiederbeck nbiederbeck commented Aug 17, 2022

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

codecov bot commented Aug 17, 2022

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.

pyirf/utils.py Show resolved Hide resolved
@RuneDominik
Copy link
Member

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.

pyirf/tests/test_utils.py Outdated Show resolved Hide resolved
@kosack
Copy link

kosack commented Oct 20, 2022

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.

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

Successfully merging this pull request may close these issues.

4 participants