Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kde_histogramdd, test need to be made more generic and have hacks removed #284

Open
jllanfranchi opened this issue Jan 9, 2017 · 2 comments

Comments

@jllanfranchi
Copy link
Contributor

  • requires specific binning dimension names
  • reordering dimensions for PID is performed in ad hoc, arcane fashion
  • energy units are forced to be GeV or it fails
  • must be 'pid' dimension and two other dimensions, not clear if those have to be 'coszen' and 'energy' or just two other dimensions, but 'energy' does have to have units of GeV if it is present, apparently
  • calls "dimensions" bins (rather than "dimensions") in code just to make it confusing
  • unit test is very poorly constructed to show what it's doing and why (silly hacks to get random numbers in a specific range? ; no repeatability since no seed values set; etc.)
  • unit test fails, but rather than just hacking it further, it should be cleaned up
@philippeller
Copy link
Collaborator

philippeller commented Mar 11, 2017

Still not a beauty, but errors in tests removed and superficially cleaned up in https://github.com/jllanfranchi/pisa/pull/289

@LeanderFischer
Copy link
Collaborator

Possibly realted to #90 and #108. But there is a lot of stuff that relies on this specific binning.. not great 😔

@LeanderFischer LeanderFischer added this to the PISA 4.2 milestone May 8, 2024
@thehrh thehrh removed this from the PISA 4.2 milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants