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

adding new io container for DL1 mono reconstruction and example notebook on how to use it #30

Merged
merged 10 commits into from
Nov 19, 2018

Conversation

vuillaut
Copy link
Member

Changes:

  • add a new io container for DL1 mono reconstruction
  • example notebook to show how to use it
  • add utils.get_event_pos_in_camera function to get the position of the source in camera using ctapipe coordinates procedures
  • add utils.disp function to compute disp (in meters) using get_event_pos_in_camera and straight out of ctapipe containers

This PR is to show how I propose to solve issues #10 and #11.
The advantages of using a custom ctapipe container are:

  • fix the names of variables for the rest of the chain
  • be able to add custom variables such as disp or mc_core_distance
  • handle units correctly
  • use ctapipe containers writing and reading procedures

If that looks ok with you, let me know but don't merge it right away, I'll use the same PR to make the actual changes in the scripts.

@rlopezcoto
Copy link
Contributor

ok, thanks @vuillaut, this will affect the way data are read in for lstpipe but I see that you already plan to include this in the PR. If @misabelber es ok with the changes for the reading, you can go ahead with the changes to the rest of the scripts and I'll merge them afterwards

Copy link
Collaborator

@misabelber misabelber left a comment

Choose a reason for hiding this comment

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

Thank you very much!! Those are changes that were very necessary but I totally failed to implement. Specially the usage of ctapipe coordinate systems! About ctapipe data containers, I wonder if they will be as easy to use with scikit-learn as pandas dataframes were?

@rlopezcoto
Copy link
Contributor

ok, so could you please write some test modules so we can be checking that the code is not being broken?
I would also like to see that the change of coordinate system is not screwing things up with the reconstruction. Whenever @vuillaut has implemented those in the new lstpipe scripts, could one of you @misabelber or @vuillaut check that things are still properly calculated (Hillas parameters...)? And that you get similar results as before?

@vuillaut
Copy link
Member Author

@misabelber The containers dump pandas dataframe in hdf5 (see last notebook cell) so it won't change the rest of the analysis.

@misabelber
Copy link
Collaborator

@vuillaut Ahh no problem then!
@rlopezcoto Of course, I can test everything when the changes are implemented.

It shows:
- what is disp and that his value is well calculated
- that the source position and disp computed with custom code and code with ctapipe coordinates system are equal
- that the custom code is 10x faster
@vuillaut
Copy link
Member Author

@rlopezcoto a question relative to the coordinates system and change from sky to camera frame.

In the last notebook I pushed, you can see that the results with custom code (from Mab) and the coordinates change with ctapipe coordinates system (based on astropy) give the same results.

However, the change of frame with astropy coordinates is quite slow. In this case the custome code appears to be about 10 times faster.
But it might be more error-prone if camera rotation changes or ctapipe coordinates are handled differently in the future.

What approach do you think we should keep by default (in any case I'd be in favor of keeping both codes in the repo for now and explain this in the docstring) ?
Thanks

The last PR removed unused files but disp.py was still imported in __init__.py
Use utils instead now.
- new custom function to make the calibration
- new function to make dl1 reduction step
- new function to make r0_to_dl1
- new function to get event type number from a particle name
- notebook to compare previous chain and new one --> get the same results
@vuillaut
Copy link
Member Author

Hi.
I finished the refactoring to use ctapipe container and data dumping.

  • new custom function to make the calibration
  • new function to make dl1 reduction step
  • new function to make r0_to_dl1
  • new function to get event type number from a particle name
  • notebook to compare previous chain and new one --> get the same results

I flag the previous get_events() chain as deprecated but did not remove it for now.
You can see in the notebook that we get the exact same results for both chains.

@rlopezcoto
Copy link
Contributor

Ok, thanks Thomas, could you please @misabelber have a look to the final code and see if you agree with all the changes before merging it?

@vuillaut vuillaut mentioned this pull request Nov 19, 2018
@misabelber
Copy link
Collaborator

I just reviewed all the changes and they seem perfect to me. Thank you @vuillaut for taking all this tasks. Also the installation package looks very good and useful.

@rlopezcoto rlopezcoto merged commit 821105a into cta-observatory:master Nov 19, 2018
@vuillaut vuillaut deleted the hdf5_files branch January 16, 2019 17:44
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