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

Interpolate psf #156

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Interpolate psf #156

merged 4 commits into from
Jun 21, 2021

Conversation

jsitarek
Copy link
Contributor

a function to interpolate PSF tables. I made it in a similar style like the other two interpolation functions. One difference is that I added an option to interpolate over cumulative distribution. In principle this could help in the binning is fine and thus event statistics are sparse. For the tests that I did I haven't seen any significant difference, but the performance might depend on a particular use case. The default is to interpolate over differential distribution (which is slightly simpler)

@maxnoe I incorporated your comments about solid cone function, please let me know if you have any further comments

@codecov
Copy link

codecov bot commented May 24, 2021

Codecov Report

Merging #156 (a5b0d4f) into master (c2cc679) will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   89.83%   90.13%   +0.29%     
==========================================
  Files          42       42              
  Lines        1594     1642      +48     
==========================================
+ Hits         1432     1480      +48     
  Misses        162      162              
Impacted Files Coverage Δ
pyirf/interpolation.py 100.00% <100.00%> (ø)
pyirf/tests/test_interpolation.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2cc679...a5b0d4f. Read the comment docs.

@jsitarek
Copy link
Contributor Author

Hi @maxnoe

can you approve the PR? as I wrote above, your comments are already implemented

stds = np.empty(n_en)
for i_en in np.arange(n_en):
w = matrix[i_en, :]
if np.sum(w) > 0:
Copy link
Member

@maxnoe maxnoe Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For weights to make sense, they have to be all >= 0, not just the sum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually there should be no negative weights, but it can happen at low energies that all bins are empty and you have 0s everywhere, this check is meant exactly for such a case

@maxnoe
Copy link
Member

maxnoe commented Jun 21, 2021

Looks good to me, sorry for the delay @jsitarek

@jsitarek jsitarek merged commit 21f6b87 into master Jun 21, 2021
@jsitarek
Copy link
Contributor Author

thx for quick feedback @maxnoe, I merged the PR

@jsitarek jsitarek deleted the interpolate_psf branch June 21, 2021 08:42
@maxnoe
Copy link
Member

maxnoe commented Jun 21, 2021

I wanted to give @HealthyPear a chance to review as well, but I think it's ok

@HealthyPear
Copy link
Member

I was about to re-start, but I understand it was open since a while...
Anyway at first sight I have nothing against - just wondering if it made sense to make 2 assertions within the same test, one for high counts and one for sparse ones, so you could check if the weights check was right

@jsitarek
Copy link
Contributor Author

Hi, sorry @HealthyPear, since there were no comments from you before in this PR I did not think that you wanted to still have a look into this. If you have any further comments, please let me know and I can make a separate PR with them.
About what you wrote above, I'm not sure if I understand it right. When I check the normalization I do an assertion with and 'or', if the bins normalize to 1 or to 0, the second case is when there is a completely empty row (because of lack of surviving MC at some energy)

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.

None yet

3 participants