search

cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
62 stars 266 forks source link

Make Hillas parameter 1 / 2 function consistent #15

Closed cdeil closed 9 years ago

cdeil commented 9 years ago

@kosack I see you've added a hillas_parameters_2 function in ctapipe.reco.

I guess it's purpose is to use Hillas parameter computation as an example for speed comparisons / teaching and we'll add C and Numba versions? So I'm +1 to keep it.

But can you make the returned parameters consistent between the different functions? It might also make sense to return a HillasParameters object. One advantage would be that it then shows up in the docs and we can document it's parameters. Another advantage is that we can add properties and convenience methods to lazily compute only things the user needs.

kosack commented 9 years ago

Yes, I intend to do that. I just want to decide on how we make structures first: should we return dicts, namedtuples, classes, etc? In the end, we want to write these to a FITS file columns, so they need to at least be "transformable" into dicts, but that is true with all of those options.

At some point we need to define data "classes" (or whatever) to keep naming consistent.

Another issue is that hillas_parameters and hillas_parameters_2 give different results (they define length and width differently, I think, and compute different parameters like the rotation angle, etc). Right now neither computs 3-rd order moments, but I think I will put that into a different algorithm, or as an option, since they are not always needed.

cdeil commented 9 years ago

+1 to use classes ... in this case I don't know if it should be HillasParameters and just represent the parameters, or if it should be HillasReco or HillasParametrisation and also expose a method to compute it.

kosack commented 9 years ago

So far, I just made a data class called MomentParameters that is a namedtuple, and I normalized the output of both methods. hillas_parameters doesn't calculate the rotation angle psi, which is needed for reconstruction, so for now it is set to None, but in version 2 it is calculated. I may just swap the two to avoid confusion. I also added a helper function to visualization.CameraDisplay to make it easy to overlay MomentParameters

cdeil commented 9 years ago

@kosack - I've added docstrings to the classes: https://github.com/cta-observatory/ctapipe/commit/e0e7c7bd20d73440182fc1899935ca3f4f54e96d

They now show up in the API docs: https://cta-observatory.github.io/ctapipe/api/ctapipe.reco.MomentParameters.html

One more thing ... are you OK with this coding guideline? https://github.com/cta-observatory/ctapipe/commit/8d5ecd68ef12ef4450d66215c1214c2fb9c3a953

kosack commented 9 years ago

Yes, that's fine with me. I actually didn't realize the "new-style" classes (inheriting from object) is now implicit in python 3, so it looks fine