Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 76.24% 77.60% +1.36%
==========================================
Files 8 8
Lines 1229 1259 +30
==========================================
+ Hits 937 977 +40
+ Misses 292 282 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| [dev-dependencies] | ||
| rstest = "0.25" | ||
| itertools = "0.14.0" |
There was a problem hiding this comment.
| itertools = "0.14.0" | |
| itertools = "0.14" |
|
|
||
| pub fn id(&self) -> usize { | ||
| self.0 | ||
| } |
There was a problem hiding this comment.
Symbol is an internal type, so you might instead just make it a struct with named fields - this is a non-breaking change. Though it sort of implies that id is mutable. That said, not a big deal. Whatever makes sense on this.
| let mut entering = Symbol::invalid(); | ||
| let mut min_id = usize::MAX; | ||
|
|
||
| for (symbol, value) in &objective.cells { | ||
| if symbol.kind() != SymbolKind::Dummy && *value < 0.0 { | ||
| return *symbol; | ||
| if symbol.id() < min_id { | ||
| min_id = symbol.id(); | ||
| entering = *symbol; | ||
| } | ||
| } | ||
| } | ||
| Symbol::invalid() | ||
| entering |
There was a problem hiding this comment.
Can this use min_by_key. E.g. something like:
| let mut entering = Symbol::invalid(); | |
| let mut min_id = usize::MAX; | |
| for (symbol, value) in &objective.cells { | |
| if symbol.kind() != SymbolKind::Dummy && *value < 0.0 { | |
| return *symbol; | |
| if symbol.id() < min_id { | |
| min_id = symbol.id(); | |
| entering = *symbol; | |
| } | |
| } | |
| } | |
| Symbol::invalid() | |
| entering | |
| objective.cells | |
| .iter() | |
| .filter(|(symbol, value)| symbol.kind() != SymbolKind::Dummy && *value < 0.0) | |
| .min_by_key(|(symbol, _)| symbol.id()) | |
| .unwrap_or(Symbol::invalid()) |
There was a problem hiding this comment.
Probably yes, this is a dirty branch, I'm just exploring what works at this moment so I want to keep the changes minimal.
|
Naive question (without looking at the details of the actual code): is there a reason why the values that are being searched can't be kept in symbol id order and then the first element is the one returned always? |
That will be the next thing I check as it seems the best option to have this sorted already (store in minheap maybe?), but I finished testing this at 1AM yesterday and needed some sleep. Will get back to this when I have some more time. |
|
I will also try applying ratatui/ratatui#1855 (comment) to kasuari. Thanks @Teufelchen1 for your work that led to this discovery! I don't see a reason why using a deterministic hasher here would be a problem, and this intuitively makes sense why it would fix the issue (but I would want to get deeper into that in free time to better understand what is happening). As a side note there is a high potential for some optimizations on ratatui side. Even after fixing this, the time complexity for that is quite bad. We can apply some optimizations for e.g. cases like layouts using only |
3 participants
Resolves #18