Skip to content

feat: use algorithms logger service in IrtCherenkovParticleID #1839

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

This PR ensures that IrtCherenkovParticleID uses the algorithms logger, so it does not have to be passed by the factory.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue: migration to algorithms)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

@github-actions github-actions bot added the topic: PID Relates to PID reconstruction label Apr 24, 2025
veprbl
veprbl previously approved these changes Apr 24, 2025
@veprbl
Copy link
Member

veprbl commented Apr 24, 2025

I've looked into this yesterday, we could pass a algorithms::detail::LoggerStream, but, unfortunately, it doesn't yet implement fmt-style printing. Other case would be to dismantle adhoc Tools.h and move printing to the algorithms. There will be a little bit of code duplication then (acceptable level, IMO).

@wdconinc
Copy link
Contributor Author

I've looked into this yesterday, we could pass a algorithms::detail::LoggerStream, but, unfortunately, it doesn't yet implement fmt-style printing. Other case would be to dismantle adhoc Tools.h and move printing to the algorithms. There will be a little bit of code duplication then (acceptable level, IMO).

I have this working locally with e.g.:

-  void Print(std::shared_ptr<spdlog::logger> m_log,
-             spdlog::level::level_enum lvl = spdlog::level::debug) {
-    m_log->log(lvl, "{:=^60}", " MergeParticleIDConfig Settings ");
-    auto print_param = [&m_log, &lvl](auto name, auto val) {
-      m_log->log(lvl, "  {:>20} = {:<}", name, val);
+  template<algorithms::LogLevel lvl = algorithms::LogLevel::kDebug>
+  constexpr void Print(const algorithms::LoggerMixin* log) {
+    log->report_fmt<lvl>("{:=^60}", " MergeParticleIDConfig Settings ");
+    auto print_param = [&log](auto name, auto val) {
+      log->report_fmt<lvl>("  {:>20} = {:<}", name, val);
     };
     print_param("mergeMode", mergeMode);
-    m_log->log(lvl, "{:=^60}", "");
+    log->report_fmt<lvl>("{:=^60}", "");
   }

All the log levels are available at compile time, never set based on runtime. We upgrade to template arg, then pass this as a const LoggerMixin pointer.

@wdconinc
Copy link
Contributor Author

I have this working locally with e.g.:

"working"

@wdconinc
Copy link
Contributor Author

This compiles now with eic/algorithms#22. protected for log(fmt, msg) was still a bit strict in this case, since the Tools are external. I didn't want to create a public logger either, so I make Tools a mix-in class that uses CRTP to properly gain access to the algorithm's logger.

Suggestions for better names for Tools are welcome.

@wdconinc wdconinc force-pushed the irt-cherenkov-particlr-id-algorithms-logger branch 2 times, most recently from 49a575d to 2a7bf7f Compare May 1, 2025 19:10
@wdconinc wdconinc force-pushed the irt-cherenkov-particlr-id-algorithms-logger branch from 66e4a11 to f0e3ea2 Compare May 2, 2025 03:28
@wdconinc wdconinc requested a review from veprbl May 2, 2025 03:35
@torrijeske
Copy link

✅ Images for pipeline 117102 are now available. View here

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are few things which move in a wrong way here, I would really prefer if Tools was moving towards becoming a namespace, instead of full fledged class (dissolving it would be a plus, but certainly outside of scope). Same goes for use of Config class, which now has to know about algorithm class.

@wdconinc
Copy link
Contributor Author

wdconinc commented May 2, 2025

There are few things which move in a wrong way here, I would really prefer if Tools was moving towards becoming a namespace

I'm on the same page: Tools needs to disappear. Same thing with the Print functions in the configs. The goal here was to not rewrite the algorithm in this PR, only how it prints. I don't know why a config needs to print (beyond having an operator<<).

I already have a branch locally that does some of that (doesn't get rid of Tools since too involved, but again here the Print functions should just become operator<<).

@wdconinc wdconinc force-pushed the irt-cherenkov-particlr-id-algorithms-logger branch from f12a566 to 92ff5ab Compare May 2, 2025 19:39
@wdconinc
Copy link
Contributor Author

wdconinc commented May 2, 2025

There are few things which move in a wrong way here, I would really prefer if Tools was moving towards becoming a namespace, instead of full fledged class (dissolving it would be a plus, but certainly outside of scope). Same goes for use of Config class, which now has to know about algorithm class.

@veprbl Tools is namespace now; some stuff taken out (maybe we can push it a level up into a detail/ directory). Probably another iwyu pass, but should be good for another review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: PID Relates to PID reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants