Implement dataset index and add PSTracks v4.2, NT v5.1

icecube / flarestack

Unbinned likelihood analysis code for astroparticle physics datasets

https://flarestack.readthedocs.io/en/latest/?badge=latest

MIT License

8 stars 7 forks source link

Implement dataset index and add PSTracks v4.2, NT v5.1 #180

Closed mlincett closed 2 years ago

mlincett commented 2 years ago

This PR:

implements a dataset index, with documentation, closes #182
adds interfaces to PSTracks v4.2 and NorthernTracks v5.1 (the two datasets can be accessed through the index);
contrarily to previous versions, NTv5.1 uses a combo data season to fix #209 (similar to PSTracks);
adds deprecation warning for PSTracks v4.0;
improves ResultsHandler handling of errors, closes #207;
let the DESY cluster use the user's python installation, fixes #181 (on WIPAC this is not required);
fixes a data path handling regression introduced with one of my previous PRs.

mlincett commented 2 years ago

Requesting a review!

Please note that the commit history is a bit chaotic due to some kind of merge/rebase mistake on my part. Will make sure to squash everything on merge.

robertdstein commented 2 years ago

Thanks for all this @mlincett! One broad comment though: this is a lot of changes which are not really all directly coupled together. As a general rule, I think one PR per issue is optimal, and here at least I'd say this could have been more easy to review if it was several independent PRs. Reviewing smaller pieces is easier than doing it all at once.

mlincett commented 2 years ago

Thanks for all this @mlincett! One broad comment though: this is a lot of changes which are not really all directly coupled together. As a general rule, I think one PR per issue is optimal, and here at least I'd say this could have been more easy to review if it was several independent PRs. Reviewing smaller pieces is easier than doing it all at once.

You are right, and I was indeed a bit worried about this. Somehow it came natural to implement one feature/fix to help with testing another. But now that everything is there I am fine with splitting this into three/four separate PRs.

robertdstein commented 2 years ago

Thanks for all this @mlincett! One broad comment though: this is a lot of changes which are not really all directly coupled together. As a general rule, I think one PR per issue is optimal, and here at least I'd say this could have been more easy to review if it was several independent PRs. Reviewing smaller pieces is easier than doing it all at once.

You are right, and I was indeed a bit worried about this. Somehow it came natural to implement one feature/fix to help with testing another. But now that everything is there I am fine with splitting this into three/four separate PRs.

I think it's too much work to split this now, but just something to bear in mind for next time

mlincett commented 2 years ago

I think it's too much work to split this now, but just something to bear in mind for next time

Uhm, I still think it will save us a lot of headaches and make the review quicker. Will split this in five different PR!