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

Build changes for COIN #1614

Merged
merged 9 commits into from Nov 9, 2023
Merged

Build changes for COIN #1614

merged 9 commits into from Nov 9, 2023

Conversation

bennibbelink
Copy link
Contributor

This makes appropriate build changes in #1613 from the upstream repo to follow our CI best practices

@bennibbelink
Copy link
Contributor Author

bennibbelink commented Nov 6, 2023

Hmm I didn't quite think this through. This PR has the appropriate build changes but without the appropriate source code changes in #1613 it will fail the cbc unit tests (it doesn't skip them anymore since --allow-milps is enabled)

@@ -243,7 +243,7 @@ TEST(ProgTranslatorTests, translation) {
EXPECT_NO_THROW(SolveProg(iface));
const double* soln = iface->getColSolution();
const double* check = checkface.getColSolution();
array_double_eq(soln, check, narcs + nfaux, "soln");
array_double_near(soln, check, narcs + nfaux, 0.00000000001, "soln");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This margin was chosen arbitrarily

Copy link
Member

@gonuke gonuke Nov 9, 2023

Choose a reason for hiding this comment

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

I think there is a cyclus definition of "small" somewhere - maybe eps? Not sure if/how it's available in testing

Copy link
Member

Choose a reason for hiding this comment

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

cy_eps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to include cyc_limits.h, now we pass in cy_eps as the margin

@bennibbelink bennibbelink marked this pull request as ready for review November 8, 2023 16:58
CHANGELOG.rst Outdated
@@ -37,6 +37,7 @@ Since last release
* fix sell_policy that was offering bids when capacity was inbetween 0 and the
quantize, bids that one was not able to fullfill and caused cyclus to crash. (#1552)
* Deprecation warnings involving <boost/detail/sp_typeinfo.hpp> (#1611)
* Segmentation faults when calling Cbc (#1614)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Segmentation faults when calling Cbc (#1614)
* Resolves segmentation faults when calling Cbc (#1614)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this, as well as the previous fix in CHANGELOG.rst that was similarly worded

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks for more cleanup @bennibbelink

@gonuke gonuke merged commit dfa159f into main Nov 9, 2023
13 checks passed
@bennibbelink bennibbelink deleted the fix-allow-milps branch November 10, 2023 00:10
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.

None yet

2 participants