search

icecube / pisa

Monte Carlo-based data analysis
http://icecube.github.io/pisa/
Apache License 2.0
19 stars 49 forks source link

honda 3D flux likely doesn't handle arbitrary binning dim order #88

Closed jllanfranchi closed 8 years ago

jllanfranchi commented 8 years ago

the 2D had some bugs as to when to do transpose and when not to. should verify that binning order does not similarly affect 3D flux.

steven-j-wren commented 8 years ago

Currently there isn't a 3D calculation at all. I can fix that. What's the bug in 2D exactly?

steven-j-wren commented 8 years ago

@3D calculation is here. https://github.com/steven-j-wren/pisa/blob/cake_HondaFlux3D/pisa/stages/flux/honda.py

However, it explicitly does NOT handle arbitrary binning yet and I added 

if self.output_binning.names[0] != 'true_energy':
    print "Arbitrary handling of dimensionality not currently supported in 3D"
    print "Please use (E,CZ,Az) in that order when calling for 3D fluxes"
    # TODO: implement this functionality
    raise NotImplementedError()

purposefully so the user can't do it. Obviously I want to fix this I'm just not sure what the equivalent transpose is in higher than 2 dimensions.

jllanfranchi commented 8 years ago

Fair enough, I agree that the "right" thing to do before a proper implementation is to raise an exception as you do here. I'll leave this open until such time we work out the 3D fluxes, but am removing any milestone since if I recall correctly the 3D flux isn't a high priority (correct me if it would be appropriate for 3.1 though).

steven-j-wren commented 8 years ago

Honestly, I don't know who will use it at all. Doesn't mean it's not worth implementing just in case (especially since it's easy) but there's absolutely no rush. At any rate it still needs an improvement to be cyclic about zero degrees.

jllanfranchi commented 8 years ago

Fixed with commit https://github.com/jllanfranchi/pisa/commit/3c7daa5779f4939dc7404fa0a67f2ecdc8c31b36 / PR #118