Skip to content

Add Sandwiching #130

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

Merged
merged 12 commits into from
Jul 7, 2025
Merged

Add Sandwiching #130

merged 12 commits into from
Jul 7, 2025

Conversation

kofgokhan
Copy link
Contributor

@kofgokhan
Copy link
Contributor Author

Not sure how properly I implemented the extension

Copy link

codecov bot commented Jun 28, 2025

Codecov Report

Attention: Patch coverage is 98.82353% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.72%. Comparing base (e5ebe9b) to head (303c51d).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
ext/MultiObjectiveAlgorithmsPolyhedraExt.jl 98.80% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   98.71%   98.72%   +0.01%     
==========================================
  Files          10       12       +2     
  Lines        1011     1101      +90     
==========================================
+ Hits          998     1087      +89     
- Misses         13       14       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@odow
Copy link
Member

odow commented Jun 29, 2025

I want to make some changes, but I'm flying home so will do later in the week. I think we should move more of the code to the extension.

@odow
Copy link
Member

odow commented Jul 3, 2025

Ready for me to take another look?

@kofgokhan
Copy link
Contributor Author

I think we need to change _distance as well. \delta_optimizer gets rebuilt whenever we want to compute the _distance. Other than that, sure.

@kofgokhan
Copy link
Contributor Author

A couple of things:

  1. Initially, the anchor points are part of the IPS but in its final state some of those anchor points can be dominated. This can be fixed by augmentation or with a second stage I believe.

  2. When we terminate due to MOI.TIME_LIMIT with 0.0 seconds like in the test, anchors are already in the IPS. So, we can either check for time limit during the computation of the anchors or we can just say even if the user wants a 0 seconds time limit they will have the anchors points to work with.

I wanted to point these things out in order to make sure that the tests we perform are solid and robust.

@odow
Copy link
Member

odow commented Jul 6, 2025

It's okay to solve some points before checking the time limit, so that even if time limit is 0 we still return with > 0 points.

There's a filter_nondominated function which can be called at the end to remove dominated points from the vector of solutions.

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

There are things we can tweak but this is good enough for now

@odow odow merged commit 23ae157 into jump-dev:master Jul 7, 2025
7 checks passed
@odow odow mentioned this pull request Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants