chore: Use new unique_ptr based SetEventHeader #1423

ChristianTackeGSI · 2023-07-07T08:33:46Z

Also update examples to current standards and compile check
them.

Also update codemeta.json.

Checklist:

Followed the Contributing Guidelines

Run meta_update.py

ChristianTackeGSI · 2023-07-07T09:47:02Z

@YanzhaoW does that look good to you?

YanzhaoW

Hi, thanks for the PR. There is no problem concerning the event header. It's ok to be merged.

However I would like to urge that we still need to specify the ownership transferring for other objects, like FileSource, FileSink, FairTask. Maybe we could have some PRs to address this issue?

YanzhaoW · 2023-07-07T11:30:15Z

examples/MQ/pixelDetector/macros/run_dAsciiSource.C

-    fRun->SetSink(new FairRootFileSink(outFile));
+    FairRunAna run{};
+    run.SetEventHeader(std::make_unique<PixelEventHeader>());
+    run.SetSink(new FairRootFileSink(outFile));


I think we should indicate the ownership transferring in our example code:

Suggested change

run.SetSink(new FairRootFileSink(outFile));

auto outFile = std::make_unique<FairRootFileSink>();

run.SetSink(outFile.release());

We already have a SetSink(unique_ptr). So you're right, I should use that!

We don't have that for SetSource yet, because SetSource is a virtual thing which we need to understand first.

I have updated the commit to use unique_ptr for FairSink. But not yet for FairSource.

Let's please postpone the SetSource thing until we understand that mess a bit better.

But fSource in the FairRun is already a unqiue_ptr:

FairRoot/base/steer/FairRun.h

Lines 243 to 245 in fa7108b

std::unique_ptr<FairSource> fSource{}; //!

Why can we not have a setter taking a unique_ptr if the FairSource is an abstract class?

Take a look at FairRunAnaProof::SetSource which overrides SetSource. I haven't even started to think on how to fix this.
And TBH I just hope for it to go away sooner than later, because we deprecated Proof in FairRoot. Then this problem is just gone.

Oh, sorry, I understood incorrectly. I didn't see SetSource is a virtual function.

This is bad. Setters and getters should not be a virtual function.

The original SetSink is virtual too, but luckily noone (in FairRoot) overrides it. We probably should remove the virtual. But as we want to deprecate it anyway…

I think the FairRun::SetSource has been made virtual only to implement FairRunAnaProof::SetSource.
I vote for removing virtual. I can make a corresponding PR.

YanzhaoW · 2023-07-07T11:32:17Z

examples/MQ/pixelDetector/macros/run_dAsciiSource.C


    PixelFindHits* hitFinderTask = new PixelFindHits();
-    fRun->AddTask(hitFinderTask);
+    run.AddTask(hitFinderTask);


Same here

Suggested change

run.AddTask(hitFinderTask);

auto task = std::make_unique<PixelFindHits>();

run.AddTask(task.release());

I have added a new commit which adds a unique_ptr based FairRun::AddTask(). It's only a new API "for the public", not yet fixing any internals (because that looks like a bigger thing).

And it only uses the new API in the examples that I touch in this PR anyway…

Maybe add another thing that came to my mind:

Instead of using make_unique (and move) every time, there is a technique, called type erasure, that can be particularly useful in this case.

Instead of

run.AddTask(std::make_unique<PixelFindHits>());

With type erasure, it's much clearer:

run.AddTask(PixelFindHits());

Can you elaborate on this? I cannot follow tbh.

Are you proposing AddTask(void*)?

How is AddTask(PixelFindHits()) keeping the temporary object alive? (And what is the function signature?)

The signature of AddTask could be:

void AddTask(Task task);

and the Task class could be defined as:

class Task{ public: template<typename T> Task(T&& obj) {} class Concept{ ... }; template <typename T> class Model : Concept{ ...}; private: std::unique_ptr<Concept> obj; };

Yeah, it's complicated for developers to implement it. But it's super easy for users to use it. For more information you can check this website or this video

The top two issues in FairRoot/ROOT(/Geant4) - from my point of view - are

unclear/messy object ownership, and

tons of tight coupling and global state (which renders testability very hard, unit testing is non-existent (in FairRoot)).

Virtual functions are really not a big issue compared to the above, but I am happy to learn and am open to arguments and evidence to the contrary :)

I have tried to understand some of this (watched first half or of the video–quite good one!).

For example I don't know, what external polymorphism really buys us? And we're currently somewhat forced by ROOT to use classic (runtime) polymorphism anyway.

As Dennis already pointed out: Our current problem is the unclear ownership. Which is a much bigger problem then using sophisticated tools ("Yeah, it's complicated for developers to implement it.") that some of our developers are not accustomed at using. And for fixing the ownership stuff, it currently feels better to be quite explicit about it.

And to be honest: I like the verbose make_unique. It's more transparent, more explicit. (So I actually do understand, why you don't like "my" "facade like" helpers. And this time I don't like the hiding of things.)

Sorry, I didn't mean we need adopt this idiom as soon as possible, but rather just want to give an information that type erasure could be used in this situation.

@ChristianTackeGSI
Actually the external polymorphism is just for making unrelated objects behave like objects with polymorphism. And type erasure idiom wraps a class around it and add more operations to make the object of that class like a normal value (copiable and movable). The biggest advantage of the type erasure (or some calls it value semantics) could exactly solve our first problem: unclear ownership. Users only need to think about the ownership when a pointer (or unique_ptr) is involved. But if they just use values, like a "type erasure" value, then the whole concept of ownership is non-existent and they don't even need to think about that. Of course, whenever it's easier for users, it's always harder for developers.

Yeah, like I said, I don't mean we should start working on this now without solving other major issues first. ;D

Cool!

So let's first work on getting the ownership checked around and fix this stuff. Then later on we can take a look at this again and improve on the then existing structure?
(And maybe a unique_ptr is type erased enough? 😉)

So that said:

Can we resolve this thread? Anything else I should fix for this PR?

@ChristianTackeGSI Yeah, I think so.

Also update examples to current standards and compile check them.

So that the callers can be more explicit about ownership, add an alternative FairRun::AddTask that takes a unique_ptr. Do not yet deprecate the old API, because it's used in way too many places. But we should update all our code to use the new API so that the new style ist adopted.

chore: Update codemeta.json

8750698

Run meta_update.py

ChristianTackeGSI force-pushed the pr/up_setevthead branch from a86076e to a5fcd82 Compare July 7, 2023 08:50

ChristianTackeGSI marked this pull request as ready for review July 7, 2023 09:46

ChristianTackeGSI mentioned this pull request Jul 7, 2023

Make member pointer fEvtHeader of FairRun into unique pointer #1254

Merged

3 tasks

YanzhaoW reviewed Jul 7, 2023

View reviewed changes

ChristianTackeGSI added 2 commits July 7, 2023 14:03

chore: Use new unique_ptr based SetEventHeader

f988b81

Also update examples to current standards and compile check them.

ChristianTackeGSI force-pushed the pr/up_setevthead branch from a5fcd82 to 1cddb87 Compare July 7, 2023 12:04

karabowi mentioned this pull request Jul 11, 2023

Remove virtual from FairRun::SetSource() #1427

Merged

1 task

karabowi self-requested a review July 11, 2023 09:35

karabowi approved these changes Jul 11, 2023

View reviewed changes

ChristianTackeGSI merged commit 0701da3 into FairRootGroup:dev Jul 11, 2023

ChristianTackeGSI deleted the pr/up_setevthead branch July 11, 2023 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Use new unique_ptr based SetEventHeader #1423

chore: Use new unique_ptr based SetEventHeader #1423

ChristianTackeGSI commented Jul 7, 2023 •

edited

Loading

ChristianTackeGSI commented Jul 7, 2023

YanzhaoW left a comment

YanzhaoW Jul 7, 2023

ChristianTackeGSI Jul 7, 2023

ChristianTackeGSI Jul 7, 2023

YanzhaoW Jul 7, 2023

ChristianTackeGSI Jul 7, 2023

YanzhaoW Jul 7, 2023

ChristianTackeGSI Jul 7, 2023

karabowi Jul 10, 2023 •

edited

Loading

YanzhaoW Jul 7, 2023

ChristianTackeGSI Jul 7, 2023

YanzhaoW Jul 7, 2023 •

edited

Loading

dennisklein Jul 7, 2023 •

edited

Loading

YanzhaoW Jul 7, 2023 •

edited

Loading

dennisklein Jul 7, 2023 •

edited

Loading

ChristianTackeGSI Jul 7, 2023

YanzhaoW Jul 7, 2023

ChristianTackeGSI Jul 7, 2023

YanzhaoW Jul 7, 2023

	run.SetSink(new FairRootFileSink(outFile));
	auto outFile = std::make_unique<FairRootFileSink>();
	run.SetSink(outFile.release());

	run.AddTask(hitFinderTask);
	auto task = std::make_unique<PixelFindHits>();
	run.AddTask(task.release());

chore: Use new unique_ptr based SetEventHeader #1423

chore: Use new unique_ptr based SetEventHeader #1423

Conversation

ChristianTackeGSI commented Jul 7, 2023 • edited Loading

ChristianTackeGSI commented Jul 7, 2023

YanzhaoW left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karabowi Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YanzhaoW Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

dennisklein Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

YanzhaoW Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

dennisklein Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianTackeGSI commented Jul 7, 2023 •

edited

Loading

karabowi Jul 10, 2023 •

edited

Loading

YanzhaoW Jul 7, 2023 •

edited

Loading

dennisklein Jul 7, 2023 •

edited

Loading

YanzhaoW Jul 7, 2023 •

edited

Loading

dennisklein Jul 7, 2023 •

edited

Loading