Implement ADR 0018 #3501
Implement ADR 0018 #3501
Conversation
jl-wynen
left a comment
jl-wynen
left a comment
There was a problem hiding this comment.
Please also add an explanation in the user guide.
I did not manage to completely follow the new logic. But I also don't see a good way to make sure that all cases are covered by tests.
| arg_dict: dict[str, int | Variable] | None = None, | ||
| /, | ||
| *, | ||
| dim: str | tuple[str, ...] | None = None, |
There was a problem hiding this comment.
Please update cpp_classes.pyi to reflect this change.
tests/binning_test.py
Outdated
| assert xyz.hist(t=4, dim='x').dims == ('y', 'z', 't') | ||
|
|
||
|
|
||
| def test_hist_by_lower_dim_coord_operates_on_slabs(): |
There was a problem hiding this comment.
What are 'slabs'? I did not see that term anywhere else.
Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>
|
@jl-wynen Given some mixed experiences in the past, I'd try to document this in the docstrings, not notebooks, unless you see a particular need for explaining this elsewhere? |
Fine by me. |
|
@jl-wynen Could you please have a look at the latest commit? It only updates the docstring of |
SimonHeybrock
force-pushed
the
implement-adr-0018
branch
from
927535c to
46f0a6e
Compare
| 2. Given M-D input data with an N-D coordinate, where N<M, we can specify `dim` to | ||
| sum over, e.g., the remaining M-N dimensions while histogramming. This is often | ||
| equivalent to not specifying `dim` and a call to `sum` after histogramming but | ||
| is more memory efficient. |
There was a problem hiding this comment.
This is also needed in nanhist, bin, group.
Also, I find it hard to follow without examples. Can you add some?
There was a problem hiding this comment.
Please see added examples!
There was a problem hiding this comment.
👍 For the examples.
I understand that you don't want to duplicate the explanation. But can you at least add a link to hist from the other functions?
There was a problem hiding this comment.
I definitely want to duplicate the explanations, please see #3501 (comment). If you consider the overall docs of hist understandable now I will do that?
There was a problem hiding this comment.
Oh, I did not see that comment. I think the docstring is good enough now.
SimonHeybrock
force-pushed
the
implement-adr-0018
branch
from
7efbfa0 to
822fe59
Compare
SimonHeybrock
force-pushed
the
implement-adr-0018
branch
from
822fe59 to
e8bed03
Compare
Fixes #3435.
Fixes scipp/essreduce#47.
I also propose to close #1846. There are most likely still some remaining issues, but the original problem description has increasingly lower utility.
Implementing this change was simple at the beginning: Consider the first few commits, and you will see that adding the
dimargument was very simple (since, e.g., the underlyingmake_binnedalready had aneraseargument, which was almost exactly that). Apart from a few minor branching changes in behavior (cd8de34#diff-d64f8ed6d2f8ccfec8c613e469f7f4824afd55241e6775f1f433d7724dbb80cb) the tests just passed.Now, for the not-so-nice part: Adding handling of the so far unsupported cases (mostly https://github.com/scipp/esssans/blob/a186d7eb7c57d2b367b097596b23b9ce425c0f35/src/ess/sans/i_of_q.py#L202-L259) and performance workarounds adds one more layer to the unfortunate stack of optimizations and handling of different cases in the Python and C++ implementations of binning and histogramming. I am not at all happy with this, and I am not confident that we will not find bugs in this (maybe not returning wrong results, but at least some optimized code branch raising exceptions in cases that should work). The entire thing is very hard to see through. We do have a good number of tests, and in many cases I saw existing tests flagging issues when I adding the new features and performance tweaks. But for now at least I am out of better ideas.
To do: