-
Notifications
You must be signed in to change notification settings - Fork 21
fix: logic for finding 'bin-edges' #3252
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
Conversation
src/scipp/html/formatting_html.py
Outdated
| if length == 2: | ||
| bin_edges.append(dim) | ||
| elif dim in ds.dims and ds.shape[ds.dims.index(dim)] + 1 == length: | ||
| elif ds.sizes.get(dim, 1) + 1 == length: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handle bin edges with extra dimensions correctly?
Since this code was written, we have also added https://scipp.github.io/generated/classes/scipp.Coords.html?highlight=is_edges#scipp.Coords.is_edges, I don't know if that helps here.
I also wonder about line 234ff, was this an incomplete solution for the same problem you are fixing here? If so, can it (the if branch) be removed, or is it not covered by you solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handle bin edges with extra dimensions correctly?
I don't understand what bin edges with extra dimensions means. Can you explain / write a short example?
I also wonder about line 234ff, was this an incomplete solution for the same problem you are fixing here?
That might be the case. Ill take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scipp supports "2D" (or higher) coords. This shows up all the time in neutron-scattering data reduction after transforming coords, e.g., from time-of-flight to wavelength, since the transform depends on the other "dimensions" (such as pixel positions). See a concrete example here: https://scipp.github.io/ess/instruments/external/powgen/powgen_reduction.html#Conversion-to-d-spacing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've marked this as a draft because for now I'm not sure what the correct behavior is.
This fix does however do the right thing for the MRE and the example @SimonHeybrock posted in the original issue.
e2bc344 to
314809f
Compare
314809f to
408042f
Compare
068bf1d to
36e76d0
Compare
Co-authored-by: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com>
36e76d0 to
e22748e
Compare
Fixes #3207.
Replaced part of the
bin-edgefinding logic with what I found in the C++ string formatting code:This does not change the alignment bolding however, because there it seems the behaviour in the html fomatting is the same as the string formatting.