icecube / flarestack

Unbinned likelihood analysis code for astroparticle physics datasets
https://flarestack.readthedocs.io/en/latest/?badge=latest
MIT License
8 stars 6 forks source link

Implementation of the KDE/photospline PSF (9yr/11yr NT samples) as spatial PDF for flarestack #333

Closed msackerm closed 7 months ago

msackerm commented 7 months ago

Features

Tests

image image

Other changes

mlincett commented 7 months ago

This is a very welcome addition.

I see the photospline dependency is handled entirely through conda, and poetry should work on top of that.

It could be nice if we could make photospline (and hence the KDE features) optional, but I am not sure whether there is an elegant way of achieving this (in the past I used to do try import / except ImportError and set a boolean switch).

I use conda for everything myself, but I am afraid it is somewhat an "expensive" requirement.

robertdstein commented 7 months ago

This is very cool! Nice to see it finally being added, I always had it in mind when writing all the spatial LLH classes.

JannisNe commented 7 months ago

This is a very welcome addition.

I see the photospline dependency is handled entirely through conda, and poetry should work on top of that.

It could be nice if we could make photospline (and hence the KDE features) optional, but I am not sure whether there is an elegant way of achieving this (in the past I used to do try import / except ImportError and set a boolean switch).

I use conda for everything myself, but I am afraid it is somewhat an "expensive" requirement.

@mlincett do you mean make it an optional dependency through poetry? That's not possible because photospline needs some pre-compiled things so it's only available through conda. I guess the idea is to set up a conda environment with conda create -f conda_env.yml and then do the poetry install inside that environment. I will add some of thet to the README.

mlincett commented 7 months ago

@mlincett do you mean make it an optional dependency through poetry? That's not possible because photospline needs some pre-compiled things so it's only available through conda.

I am aware of that. What I did in other circumstances is, in case of a missing "external" dependency, to catch the "ImportError" and add a gracious failure mode for when the downstream code tries to use the classes that require the dependency.

Yet this is not very elegant and can be a pain to maintain (especially in an heavily structured code like flarestack). If no one else is concerned with the conda / photospline dependency, I am fine with taking this step as well.

JannisNe commented 7 months ago

Seems like there is some issue with urllib3 v2.0.7 and python 3.10. I pinned v2.0.6 for now. @mlincett good to merge?