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

Implement IRF interpolation method #81

Merged
merged 14 commits into from
Aug 17, 2022
Merged

Implement IRF interpolation method #81

merged 14 commits into from
Aug 17, 2022

Conversation

jsitarek
Copy link
Collaborator

@jsitarek jsitarek commented Jul 1, 2022

this still does not work too good. I will check if there is no problem with interpolation. Nevertheless @YoshikiOhtani any feedback on the code is very much appreciated.

Julian Sitarek added 2 commits July 1, 2022 10:49
energy range of IRFs had to be increased, because the code fails if there are some events not covered by the bins
modified slightly the regular expression to find the input gamma files, since they have no "gamma" in LSTProd2
relaxed the limit for running over the files with viewcone, since the test sample is created as 0.4 deg viewcone
@jsitarek jsitarek marked this pull request as draft July 1, 2022 11:24
@YoshikiOhtani YoshikiOhtani marked this pull request as ready for review August 1, 2022 13:25
@YoshikiOhtani YoshikiOhtani changed the title Irf interp Implement IRF interpolation method Aug 1, 2022
@YoshikiOhtani
Copy link
Collaborator

YoshikiOhtani commented Aug 2, 2022

Apologies for the late feedback but thank you very much for implementing! I checked the code and in general I agree with the procedure. However, I have an idea of making it more logical and smooth, and since Julian is taking holidays I pushed some commits to the branch instead of only commenting. What I mainly did is to implement the function load_irf_files not only to load the input IRF files but also to check the consistency of the configuration, such as the binning of the IRF data and the settings stored in the headers. I also changed the code so that loading of the files, interpolating the IRFs and create the HDUs are performed inside the dl2_to_dl3 function. The interpolation of the dynamic theta cuts (= rad_max) is also implemented by using the same function scipy.interpolate.griddata as the other interpolation methods. The argument --input-file-irf is now replaced with --input-dir-irf, and even if only one IRF file is input the script works by specifying the irf_interpolation_method setting to "nearest".

@aleberti since I also pushed the commits as a developer, could you please have a look at this as a reviewer when you have time? Thanks in advance!

@aleberti
Copy link
Collaborator

aleberti commented Aug 2, 2022

Hi @YoshikiOhtani, sure I can review. Maybe it would be good if you run the new interpolations on a dataset you already have and check the performance (e.g. how it compares to old methods/no interpolation or if you see strange features)? However I do not know if you have data locally, maybe you have everything in the IT container which is currently down.

@YoshikiOhtani
Copy link
Collaborator

Hi @aleberti, thanks! Exactly, I did not transfer any data to other machines, except only a DL2 MC data file produced with a fixed pointing direction that I have in my local PC. I tested creating the IRFs with the data and the create_irf script of this branch and it succeeded. Also, using the interpolation functions with the one IRF file and the "nearest" method worked fine as before implementing the interpolation methods, so I think the functionality of the script is safely protected. But for testing "linear" and "cubic" options more IRF files are needed, which I don't have now...

@aleberti
Copy link
Collaborator

aleberti commented Aug 4, 2022

Regarding this PR, should it be merged now or should we wait for Julian's opinion? Actually I will see him next week at TeVPA (he is in the participant list), so I can ask him what he thinks about it.

@YoshikiOhtani
Copy link
Collaborator

Hi, it is not urgent, so it's great if you could ask him about this next week. Then if he thinks OK let's merge to the master.

Copy link
Collaborator Author

@jsitarek jsitarek left a comment

Choose a reason for hiding this comment

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

Hi @YoshikiOhtani
sorry for a late reply.
I went through the updated PR and put a few more comments. Other than this, it is fine with me.

@YoshikiOhtani
Copy link
Collaborator

Hi @jsitarek, sorry for being late to reply, but thank you very much for your checks. I have just pushed some commits to fix them. Regarding the problem of azimuth 2 pi turn I just added FIX ME for now, but I may push a commit to fix it later. So let's merge this to the master now. If you face some problems please let me know.

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.

3 participants