Skip to content

fix issue in the single solution rcsp #368

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 3, 2024
Merged

fix issue in the single solution rcsp #368

merged 5 commits into from
Apr 3, 2024

Conversation

andrea-cassioli-maersk
Copy link
Contributor

@andrea-cassioli-maersk andrea-cassioli-maersk commented Mar 22, 2024 •
edited
Loading

Refine the RCSP logic to address the issue

#363

  • sort node labels in the post processing to make sure the path returned is the least cost instead of the first discovered.
  • there are no logic changes in the algorithm itself
  • there are no changes when rcsp finds all pareto dominated solutions
  • add a test to check the results of the fix
  • refine the git ignore to filter out some test and cmake artefacts

@andrea-cassioli-maersk andrea-cassioli-maersk marked this pull request as ready for review March 22, 2024 11:02
@andrea-cassioli-maersk andrea-cassioli-maersk mentioned this pull request Mar 22, 2024
Closed
jeremy-murphy
jeremy-murphy reviewed Apr 2, 2024
View reviewed changes
test/rcsp_single_solution.cpp Outdated
Comment on lines 104 to 107
const auto s = boost::add_vertex("s", graph);
const auto B = boost::add_vertex("A", graph);
const auto t = boost::add_vertex("B", graph);
const auto A = boost::add_vertex("t", graph);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to mislabel them? Does it affect the correctness of this test?

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, good catch! The labels were wrong, but the graph overall correct. Fixed.

@jeremy-murphy
Copy link
Contributor

This is a great contribution, thank you. Please check your test again based on my comment about the labels, make sure that it's passing because of the new sort code. I.e., make sure that it fails if you comment the sort code out. Once you've confirmed that it's all good, I'll merge it.

@jeremy-murphy jeremy-murphy self-assigned this Apr 3, 2024
@andrea-cassioli-maersk
Copy link
Contributor Author

This is a great contribution, thank you. Please check your test again based on my comment about the labels, make sure that it's passing because of the new sort code. I.e., make sure that it fails if you comment the sort code out. Once you've confirmed that it's all good, I'll merge it.

Just verified: commenting out the sorting I get:

====== BEGIN OUTPUT ======
rcsp_single_solution.cpp(116): test 'solution_resource_container.cost == 5' failed in function 'int main()'
1 error detected.

EXIT STATUS: 1
====== END OUTPUT ======

@jeremy-murphy jeremy-murphy merged commit 158eebd into boostorg:develop Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremy-murphy jeremy-murphy jeremy-murphy left review comments

Assignees

@jeremy-murphy jeremy-murphy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrea-cassioli-maersk @jeremy-murphy