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

Improved error reporting for greedy solver #539

Merged

Conversation

SylvainJoube
Copy link
Contributor

@SylvainJoube SylvainJoube commented Apr 14, 2024

A few tings:

  • Disabled info messages from the greedy solver to avoid distracting the user from important warning and error messages.
  • Added an error message when every measurement.measurement_id equals zero:

ERROR: @greedy_ambiguity_resolution_algorithm: Measurements must have unique IDs. But here, each measurement has 0 as ID (measurement.measurement_id == 0). This may be solved by loading measurement_id from the appropriate file, or by assigning a unique ID to each new measurement during CCA.

  • Added a warning message when too many measurements share the ID 0. It now says, for example:

"WARNING @greedy_ambiguity_resolution_algorithm: 14% of input measurements have an ID equal to 0 (measurement.measurement_id == 0). This may be suspicious."

  • Fixed a minor typo.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

A little later I'll very much want to review / revise what we do for log messages in all of these algorithms. But for now I'm happy with printing at least fewer messages.

We should just think of whether "flood" messages, with INFO: message lines are the things that we want to go for in the meanwhile. 🤔

Comment on lines 73 to 76
static std::ostream& flood() {
std::cout << "@greedy_ambiguity_resolution_algorithm: ";
std::cout << "INFO: @greedy_ambiguity_resolution_algorithm: ";
return std::cout;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm generally not the biggest fan of all of these output messages. 🤔 But as long as we use them, why not name this as either a DEBUG or VERBOSE output level? To be a bit more in line with Athena / Python / etc. message logging conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review :)

I made a mistake, I meant to say FLOOD: for "flood" messages. I completely agree with you, I think it's much better to have a common vocabulary and not confuse developers with too many different names referring to the same concept. I’ve changed FLOOD into DEBUG (since I only used these messages for debugging purposes). They are here to help me in case anything goes wrong and I need to debug the code, but I agree, the fewer messages we print on the terminal, the better, and there is no point in repeatedly saying "I'm doing this and that, everything works fine", better save space for the really important messages.

@SylvainJoube SylvainJoube force-pushed the improve-greedy-solver-error-handling branch 3 times, most recently from 6a217d3 to 1b7f7c8 Compare April 17, 2024 18:54
@SylvainJoube SylvainJoube force-pushed the improve-greedy-solver-error-handling branch from 1b7f7c8 to 8ea55b7 Compare April 17, 2024 18:55
@SylvainJoube SylvainJoube force-pushed the improve-greedy-solver-error-handling branch from 8ea55b7 to 241b9c7 Compare April 17, 2024 18:58
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

👍

@krasznaa krasznaa merged commit 46d86fc into acts-project:main Apr 19, 2024
17 checks passed
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.

2 participants