Conversation
pluflou
reviewed
Mar 27, 2025
| solver=solver, | ||
| verbose=verbose, | ||
| ) | ||
| if solver is not None: |
Collaborator
There was a problem hiding this comment.
Is there a reason this isn't going in the _cvx_signal_decompositions module to follow the others in this module? (by this, I mean the decomposition logic)
| verbose=verbose, | ||
| ) | ||
| if solver is not None: | ||
| if solver.upper() not in ["MOSEK", "CLARABEL", "OSQP"]: |
Collaborator
There was a problem hiding this comment.
Should we alert the user here that CLARABEL is being used if, for example, QSS was passed?
Collaborator
Author
There was a problem hiding this comment.
We should remove OSQP as well
bmeyers
commented
Mar 28, 2025
| return test_r, train_r, best_ix, jmp_counts, min_jumps | ||
|
|
||
| def plot_weight_optimization(self): | ||
| _fig, _ax = plt.subplots(nrows=3, sharex=True, figsize=(10, 5)) |
Collaborator
Author
|
@pluflou all checks are passing again, so I think we're ready for the final review |
pluflou
approved these changes
Mar 28, 2025
cocosrv
pushed a commit
to cocosrv/solar-data-tools
that referenced
this pull request
Nov 5, 2025
Capacity change algorithm rewrite, closes NatLabRockies#192
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #192
This PR addresses the bug that was uncovered where the capacity change algorithm returns incorrect results on some test data when using the default solver. This is a rewrite of the signal decomposition implementation, such that the exact some problem is now being solved by clarabel and mosek.
Checklist for PR authors (skip items if you don't have permissions or they are not applicable)