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

issue996 #172

Merged
merged 10 commits into from
Sep 7, 2023
Merged

issue996 #172

merged 10 commits into from
Sep 7, 2023

Conversation

ClemensBuechner
Copy link
Contributor

No description provided.

if (initial_landmark_graph_has_cycle_of_natural_orderings) {
log << "Found a cycle of natural (or stronger) landmark orderings. By "
<< "the definition of natural orderings, the problem is unsolvable."
<< endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I would want to make this output here. I suggest just returning infinity, but possibly including an output along the lines of this in the code that computes the flag. I find output in a "preprocessing" stage more palatable than "inside" the heuristic computation. In general, at least in future code, I'd also always guard output like this with a verbosity test, but I didn't check how we do it with other landmark-related outputs.

Copy link
Contributor Author

@ClemensBuechner ClemensBuechner Sep 7, 2023

Choose a reason for hiding this comment

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

Sure, I'll remove the comment in the heuristic. You write "including an output along the lines of this", so is there something in particular you would change about the output?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be briefer, but I don't feel strongly about this. I might just write something along the lines of "Found a cycle of natural landmark orderings." and nothing more. That the problem is unsolvable as a consequence will be printed anyway by the search algorithm, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, sure, makes sense.

@ClemensBuechner ClemensBuechner merged commit 5ecdaef into aibasel:main Sep 7, 2023
12 checks passed
@ClemensBuechner ClemensBuechner deleted the issue996 branch September 7, 2023 15:37
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