Use correct output dtype in event centric arithmetic #3278

jl-wynen · 2023-10-05T14:19:50Z

SimonHeybrock · 2023-10-10T04:52:14Z

lib/core/include/scipp/core/element/event_operations.h

-        map_and_mul_detail::args<float, double, double, double>,
        map_and_mul_detail::args<float, double, double, float>,
        map_and_mul_detail::args<double, float, float, double>,
        map_and_mul_detail::args<double, time_point, time_point, double>,
        map_and_mul_detail::args<double, time_point, time_point, float>,
-        map_and_mul_detail::args<float, time_point, time_point, double>,
-        map_and_mul_detail::args<float, time_point, time_point, float>>,
+        map_and_mul_detail::args<float, time_point, time_point, double>>,


Why have some combinations been removed?

They dealt with type promotions which are now handled in Python.

What about the inplace variants (which you did not change)?

In place operations cannot change the dtype, can they?

No they cannot, but how is that related to the code and my question?

Which in-place variants do you actually mean? This is only used by dataset::buckets::scale which is always in-place.

For example __imul__ in bins.py, right under the functions you changed in this PR.

So unless there is some other check, this used to be able to multiply double weights onto float events and convert the doubles to floats. This is no longer possible with this changes.
We allow this in other operations, so I guess I should revert this change.

And I actually removed the wrong one for the time_point overload.

SimonHeybrock · 2023-10-10T04:54:58Z

src/scipp/core/bins.py

    def __mul__(self, lut: lookup):
-        copy = self._obj.copy()
+        target_dtype = (
+            scalar(1, dtype=self.constituents['data'].dtype)


I feel we should add self.dtype, similar to self.unit?

Can you check how expensive constituents is? I think it used to unzip the indices (allocating memory), but now it probably just returns 2 stride-2 variables for begin and end, i.e., is cheap?

Well, self.unit is implemented in terms of constituents. So unless we implement dtype in C++, we'd still pay the cost.
But I agree that it would be nicer to have a dtype property on Bins.

And yes, it does unzip the indices.

Are you sure? It looks like it is handled via strides:

I don't know what you want to say with this. But here is the implementation:

scipp/lib/python/bins.cpp

Lines 84 to 93 in 92717a5

template <class T> py::dict bins_constituents(const Variable &var) {

auto &&[indices, dim, buffer] = var.constituents<T>();

auto &&[begin, end] = unzip(indices);

py::dict out;

out["begin"] = std::forward<decltype(begin)>(begin);

out["end"] = std::forward<decltype(end)>(end);

out["dim"] = std::string(dim.name());

out["data"] = std::forward<decltype(buffer)>(buffer);

return out;

}

Probably unzip does this under the hood, i.e., it does not cause copies?

It looks like it doesn't make a copy: (this messed up the binned variable, but hey ho)

import scipp as sc da = sc.data.binned_x(10, 3) print(da.bins.constituents['begin']) da.bins.constituents['begin'][0] = 1 print(da.bins.constituents['begin'])

->

<scipp.Variable> (x: 3) int64 <no unit> [0, 7, 7] <scipp.Variable> (x: 3) int64 <no unit> [1, 7, 7]

👍 ... then this is reasonably fast, i.e., usable for unit and dtype.

jl-wynen requested a review from SimonHeybrock October 5, 2023 14:19

jl-wynen force-pushed the fix-dtype-event-centric-arithmetic branch from c8d6480 to baefdc9 Compare October 5, 2023 14:29

SimonHeybrock reviewed Oct 10, 2023

View reviewed changes

jl-wynen added 4 commits October 10, 2023 10:49

Use correct output dtype in arithmetic with Bins

9676523

Remove obsolete dtype combinations

930219e

Add release note

84f95cb

Add Bins.dtype

f6f19ad

jl-wynen force-pushed the fix-dtype-event-centric-arithmetic branch from 736d98f to e2a26fe Compare October 10, 2023 08:49

Restore downcasting overloads

f95556d

jl-wynen force-pushed the fix-dtype-event-centric-arithmetic branch from e2a26fe to f95556d Compare October 10, 2023 08:50

SimonHeybrock approved these changes Oct 10, 2023

View reviewed changes

jl-wynen enabled auto-merge October 10, 2023 13:37

Merge branch 'main' into fix-dtype-event-centric-arithmetic

7416d00

jl-wynen merged commit d8197f3 into main Oct 10, 2023

jl-wynen deleted the fix-dtype-event-centric-arithmetic branch October 10, 2023 13:51

	template <class T> py::dict bins_constituents(const Variable &var) {
	auto &&[indices, dim, buffer] = var.constituents<T>();
	auto &&[begin, end] = unzip(indices);
	py::dict out;
	out["begin"] = std::forward<decltype(begin)>(begin);
	out["end"] = std::forward<decltype(end)>(end);
	out["dim"] = std::string(dim.name());
	out["data"] = std::forward<decltype(buffer)>(buffer);
	return out;
	}

Use correct output dtype in event centric arithmetic #3278

Use correct output dtype in event centric arithmetic #3278

Uh oh!

Conversation

jl-wynen commented Oct 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jl-wynen Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jl-wynen Oct 10, 2023 •

edited

Loading