search

cta-observatory / iact_event_types

BSD 3-Clause "New" or "Revised" License
4 stars 7 forks source link

Add classification option and other improvements #16

Closed orelgueta closed 3 years ago

orelgueta commented 3 years ago

Add classification option, some restructuring, one bug fix in partitioning of events for regressions and various other improvements. Won't wait for review for too long this time =)

TarekHC commented 3 years ago

Hi @orelgueta

I finally found the time to actually take a look to the new structure of the repo! (sorry I took so long!). The code is actually super well documented, so perhaps we could use sphynx? I actually never implemented it myself... But I'm sure it would be easy taking a look at how you documented every function.

I have few suggestions before merging:

I'll also try to comment inline on some of the files I found weird things.

Best, Tarek

orelgueta commented 3 years ago

I finally found the time to actually take a look to the new structure of the repo! (sorry I took so long!).

Passive aggressive works with you I see =)

The code is actually super well documented, so perhaps we could use sphynx? I actually never implemented it myself... But I'm sure it would be easy taking a look at how you documented every function.

Yeah, I wrote it in such a way that it's easy to introduce sphynx later. It's fairly easy to do, but it takes a bit of time which I prefer to invest a little later rather than now.

  • Could you please add to the current README.md a description of how to use the repo?

On my to-do list. If you insist it should happen before merging, I will (but briefly).

  • Better organize the package: I would differentiate scripts (executing the code, probably within a "scripts" folder) from libraries (classes/functions within the event_types folder?). Probably all scripts within the repo can be moved to a "scripts" folder, and the notebooks to a "notebooks" folder.

So I started doing this, but did not finish. Partly cause I first wanted your permission to remove the notebooks. They were useful at the time, but they are not really necessary anymore. We can write new ones, but those should then make use of the library.

  • Divide the content of event_types.py (it has way too many functions!): I see functions related to plotting (which could go to a plotting.py?), other functions related to train/testing (learn.py?), and others for reading/saving data (parse_data.py?).

Is it really too long? I can divide it to plotting and parse_data probably, but I wouldn't go further than that.

Cheers,

Orel

I'll also try to comment inline on some of the files I found weird things.

Best, Tarek