Enabling Plan mode on pyvroom#129
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
=======================================
- Coverage 80.3% 79.5% -0.8%
=======================================
Files 29 29
Lines 1669 1902 +233
Branches 147 149 +2
=======================================
+ Hits 1341 1513 +172
- Misses 314 377 +63
+ Partials 14 12 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| 5 48 job 1102 3 1717 | ||
| 6 48 job 2204 2 1616 | ||
| 7 48 end 2204 2 <NA> | ||
| >>> groups = solution.routes[solution.routes.type == "job"].groupby("vehicle_id")["id"].apply(lambda x: set(x.dropna().astype(int))) |
There was a problem hiding this comment.
There are two possible solutions:
- Vehicle 47: jobs in order 1414, 1515 (was 1515, 1414), with arrivals 0, 2104, 4207 for the two jobs.
- Vehicle 48: jobs in order 1616, 1717 (was 1717, 1616), with arrivals 0, 1102, 2204 for the two jobs.
Cost stays 6411; the solver is just returning a different (equivalent) route order.
This makes the doctest fail on ci if I write one solution here and the solver returns another one.
The change here make it work in both cases
There was a problem hiding this comment.
So the README exists primarily for domonstration purpouses. If the example is ambiguous, I think the right correction is to change the example so the ambiguity doesn't arrises, not to do check for multiple solutions.
|
Fantastic work, @blegat! Over at #131 I updating the code to work with vroom version 1.15. Most of the efforts has gone into getting the broken CI to work again. To my delight, the codecov-part was already fixed in this PR, so I have copied it over. I have also taken the liberty to take your glpk patch here and added it to #132. From the release it is clear that something is wrong with the Conan build. Thoughts on why is appriciated, that is not my strong suite. I have made a release without the Windows binaries though. You can check it out by installing |
| conan profile update "settings.compiler.version=17" default | ||
| conan config set "storage.path=$env:GITHUB_WORKSPACE/conan_data" | ||
| conan install --build=openssl --install-folder conan_build . | ||
| conan install --build=missing --install-folder conan_build . |
There was a problem hiding this comment.
Out of curiosity, what does this change do?
There was a problem hiding this comment.
With --build=openssl it always build openssl even if it is available from the cache while with missing it will first check the cache. We can try changing it back to see
| assert solution.unassigned == [] | ||
|
|
||
| routes = solution.routes | ||
| import pandas # used for DataFrame; import here to avoid collection-time dependency |
There was a problem hiding this comment.
why are we want to avoid collection-time dependency?
There was a problem hiding this comment.
It was to avoid having other tests fails if pandas is not available, but since it is we can move it back to line 3
|
I've merged in all the v1.15 changes now into main. I'll wait a little bit before releasing 1.15.0. Getting support for glpk and plan mode would be a welcomed addition. This is your PR, so up to you what you want to do:
If you are interested, I can also give you developer status. It gives you the ability to make development tag releases. |
|
Yes, I'm interested in the dev status, thanks :) |
jonathf
left a comment
jonathf
left a comment
There was a problem hiding this comment.
Before merging, can we just un-muffle the test you have made?
| that have predefined steps matching the optimal solution. Jobs must be | ||
| added before vehicles so that vehicle steps can reference job ids. | ||
| """ | ||
| import pytest |
@jcoupey can we fix dev-status for @blegat? Regarding the runner. Because releases takes over an hour to compile, I have limited the runner to only do full runs on explicit git tags. I've been tagging your builds along the way with And apropos releases: After merging this PR I think we are ready to release v1.15.0 of pyvroom. |
|
@blegat you should have received an invite granting you "write" rights.
@jonathf please note that v1.15.0 is not actually released upstream (we only have release candidates so far), so you probably want to wait just a bit more. I plan to tag the new version upstream very soon. |
|
Thank you for pointing this out @jcoupey. I see now it is tagged rc.2. We will do the same for pyvroom. Please let us know when upstream vroom is fully released. |
|
sorry for dropping in with absolutely nothing to contribute, but it's itching so hard: what exactly is "un-muffle the test" @jonathf ? 🤣 that sounds so funny to german ears! "muffle" seems to mean "cover smth". I'm not sure that's relating? or I'm missing that from my vocabulary? anyways, also: thanks for carrying this long awaited feature over the finish line @blegat :) |
Co-authored-by: Jonathan Feinberg <jonathf@users.noreply.github.com>
There are multiple definitions of the word muffle. In this context I intended it to me "to wrap or pad with something to dull the sound" and use the simily that testing failure is a type of alarm. I am not sure how common this frase is globally, but people I know locally knows it. |
Co-authored-by: Jonathan Feinberg <jonathf@users.noreply.github.com>
|
Thanks for giving me the dev rights, I created a tag to the last commit to make sure everything works. I pushed the branch directly to this remote and not my fork for the tag to have effect. Let's continue on #134 |
@jonathf I just tagged v1.15.0 upstream. |
5 participants
Rebased #84, fixed conflicts, added a tests and tried to fix Mac OS