Skip to content
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

Moved exceptions from init and added debug statements to process #1565

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

Conversation

simonge
Copy link
Contributor

@simonge simonge commented Aug 13, 2024

Briefly, what does this PR introduce?

Highlighted by @ShujieL and @wdconinc https://chat.epic-eic.org/main/pl/uiei8z865tbdjn4mpou19o4tuo. Failing to initialise the FarDetectorTrackerCluster algorithm, due to missing detectors in the geometry, calls an exception meaning JANA tries to init it again on the next event resulting in errors printed for each event.

This PR tries to tidy up the output by removing the exception so that the initialisation error is only printed once. A check is instead moved to the start of process() where repeated warning messages can still be accessed by setting the logger to debug.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • 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?

NA

Does this PR change default behavior?

Fewer repeated error statements should be printed. Otherwise no.

Copy link

sonarcloud bot commented Aug 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

You could use m_method == nullptr in FarDetectorMLReconstruction and m_id_dec == nullptr in FarDetectorTrackerCluster instead of adding a new initialization variable. I'll defer to @veprbl if that's better style or not :-)

// Check if the algorithm has been initialized properly
if(!m_initialized){
debug("Initialization did not complete");
return;
Copy link
Member

Choose a reason for hiding this comment

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

Unless you throw here, the Process will be called over and over.

Suggested change
return;
throw std::runtime_exception("Initialization did not complete");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will it not call process over and over anyway? I thought the behaviour we wanted was to flag the error but still return an empty collection so that the reconstruction can be carried out with the limited geometry provided. It would be nicer if upon throwing the exception in init, JANA knows not to try again.

Copy link
Member

Choose a reason for hiding this comment

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

It will not. The exceptions in Process trickle into our JEventProcessorPODIO, where we try to print error messages only once, which is the desired effect. We could do some accounting to disable collections, but, you are right, they don't get disabled from being called currently.

@veprbl
Copy link
Member

veprbl commented Aug 13, 2024

This solves a global issue locally. We should figure out why exceptions in Init are swallowed silently. I like that this removes use of JException in algorithms, and overall it's fine to take this PR in, IMO.

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

Successfully merging this pull request may close these issues.

3 participants