Conversation
|
Initial thoughts on this PR:
|
|
@idelder this is waiting on the second point from @jsiirola's comment to be addressed.
So your change to |
…he keys in their insertion order to avoid a very slow key sort Signed-off-by: Davey Elder <iandavidelder@gmail.com>
84276e1 to
0284e67
Compare
|
Okay done. The sparse_values() and sparse_items() calls get values and items via a keys() call anyway, so by moving the fix into the keys() function they automatically follow the same order. Some testing of calls to sparse_keys() below, for an nxn sparse param with 100 randomly ordered diagonal entries: Before this fix ~ O(N²) (gave up after n = 100,000)
After this fix
Checking orderingChecking 10_000 x 10_000 param for matching order... (Check all 100 but print first three entries only) PASS: all three iterators return elements in the same order. Testing script |
jsiirola
left a comment
There was a problem hiding this comment.
I have been debating this, and I think I agree this is a reasonable initial approach. You do need to update the condition and resolve the failing test (I believe that the test should be updated to iterate over a list (i.e., list(m.b)).
We should consider adding additional special-case handling for ORDERED_INDICES, too (but that can be a separate PR).
| # IndexedComponent): we might as well just sort the sparse | ||
| # _data keys instead of iterating over the whole index. | ||
| return iter(sorted_robust(self._data)) | ||
| elif SortComponents.UNSORTED in sort: |
There was a problem hiding this comment.
This needs to be either
elif sort == SortComponents.UNSORTED:or
elif not sort:
(SortComponents.UNSORTED is 0, so SortComponents.UNSORTED in sort is always True)
There was a problem hiding this comment.
I see okay that makes sense. I went with elif not sort
I'd also noticed the test_flatten bug but wasn't sure if I should mix that into this PR. Did that too.
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3971 +/- ##
=======================================
Coverage 90.14% 90.14%
=======================================
Files 917 917
Lines 109261 109263 +2
=======================================
+ Hits 98494 98496 +2
Misses 10767 10767
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes # .
Fix a very slow call from sparse_keys() to keys() for unsorted params.
Summary/Motivation:
The sparse_keys() function was updated to return ordered/sorted sparse indices when sorting was specified, going via the keys() function. The change, however, also routed unsorted parameters through the keys() function, collapsing to the final block where filtering was performed over the entire index cross product, keeping only sparse keys, but in index-insertion order. For very large but sparsely indexed params, this dramatically increased (by many orders of magnitude) the time to return the sparse keys set. For unsorted parameters, it makes sense to default back to old behaviour of just returning the unordered sparse keys immediately.
Changes proposed in this PR:
AI-Use Disclosure
or
AI tools contributed to the development of this PR
Review process (select ONE):
Notes for reviewers (optional):
Does this correctly cover the different cases of sorting?
(This is my first PR to the pyomo codebase, forgive me if I am missing something)
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: