IRLS #1263
IRLS #1263
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1263 +/- ##
===========================================
+ Coverage 90.53% 90.60% +0.06%
===========================================
Files 132 133 +1
Lines 14219 14346 +127
===========================================
+ Hits 12873 12998 +125
- Misses 1346 1348 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
j-c-c
force-pushed
the
IRLS
branch
2 times, most recently
from
58af01b to
92c14f7
Compare
There was a problem hiding this comment.
Sorry for the delay getting this back to you. Not too much to do, it looks like you were able to get a lot of code re-use from prior work so I guess that paid off :).
Some of the items here we chatted about. For those just add a short response or code comment if necessary so it's documented with the PR/code. Thanks
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class CommonlineIRLS(CommonlineLUD): |
There was a problem hiding this comment.
Since this is a form of LUD, should that be part of the name? If I understand correctly, CommonlineLUD uses the SDP solver, while this uses IRLS.
There was a problem hiding this comment.
Maybe. Yes, it is a form of LUD. Figuring out what to call these classes will be part of the project to reorganizing all of the commonline classes.
CommonlineLUD uses an implementation of ADMM with or without a spectral norm constraint to solve the LUD problem. CommonlineIRLS uses an SDP solver when there is no spectral norm constraint and ADMM when there is a spectral norm constraint to solve the least squares problem for each reweighting iteration.
| A, b = self._sdp_prep() | ||
| for _ in trange(self.num_itrs, desc="Performing iterative re-weighting."): | ||
| S_weighted = weights * self.S | ||
| gram = self._compute_gram_matrix(S_weighted, A, b) |
There was a problem hiding this comment.
What's the difference between _compute_gram_matrix and _compute_Gram? Why is one used with spectral norm constraint and one without?
There was a problem hiding this comment.
_compute_gram_matrix is calling the SDP solver in CommonlineSDP and _compute_Gram is calling the ADMM implementation in this file.
Maybe these should be renamed to indicate this. What do you think of _compute_gram_SDP and _compute_gram_IRLS?
|
|
||
|
|
||
| class CommonlineLUD(CLOrient3D): | ||
| class CommonlineLUD(CommonlineSDP): |
There was a problem hiding this comment.
This is kind of weird, no?
There was a problem hiding this comment.
Yes, it's a little weird. CommonlineLUD does use at least one method from CommonlineSDP (ie. _deterministic_rounding) and passes _compute_Gram and _construct_S to CommonlineIRLS via class hierarchy. It might be better for those methods to be utility functions that can be called independent of class hierarchy. This is another topic that I planned on addressing with the overall commonline class restructure.
There was a problem hiding this comment.
It might be better for those methods to be utility functions that can be called independent of class hierarchy. This is another topic that I planned on addressing with the overall commonline class restructure.
Right. I think that might be a better approach.
|
I think this was good to go after the name update, fire when ready :) 🚀 |
Port of the common lines method using IRLS.