Filter calibrations out of constraints summary table #4557
Conversation
| .setEnableSorting(false) | ||
| ) | ||
| // Memo rows | ||
| rows <- useMemo((props.constraintList, props.calibrationObservations)): (cl, calibs) => |
There was a problem hiding this comment.
The only real change is the calculation of the rows.
BundleMonUnchanged files (8)
Total files change -6B 0% Final result: ✅ View report in BundleMon website ➡️ |
| ScalaFnComponent[Body]: props => | ||
| for { | ||
| ctx <- useContext(AppContext.ctx) | ||
| cols <- useMemo(()): // Cols never changes, but needs access to props |
There was a problem hiding this comment.
Unrelated to the PR: in other places where we have stable columns definitions, we have moved them to a val with .reuseAlways, which declutters the hook section.
There was a problem hiding this comment.
Yeah. I was just lazy here. :( I'll move it.
There was a problem hiding this comment.
I can't really move it to a val since it needs the props, but I can have the memo call a function.
There was a problem hiding this comment.
Oh right, it needs the programId.
| cl | ||
| .map: (obsIdSet, cs) => | ||
| (obsIdSet -- calibs) | ||
| .map: filtered => | ||
| ConstraintGroup(cs, filtered) |
There was a problem hiding this comment.
Why not filter in the caller rather than passing the filtered observations list all the way up to here?
There was a problem hiding this comment.
I guess I was lazy again and just followed existing precedent. We do this for all the kinds of groups we have. I wonder if there should just be a lazy val in programSummaries that does this?
There was a problem hiding this comment.
filtered observations list
Do you mean the calibration observations list?
There was a problem hiding this comment.
Yeah, I should have said "list of observations to be filtered out".
There was a problem hiding this comment.
I wonder if there should just be a lazy val in programSummaries that does this?
Sounds good to me
There was a problem hiding this comment.
This was a good call because it turns out that we don't want the calibration observations in any of the groupings that we calculate in the ProgramSummaries. We were filtering them out way down the chain and not always completely. Which messed up, for example, selections in some of the trees because the calibration obs ids were not being filtered out of the tree representation.
toddburnside
force-pushed
the
sc-4733-constraints-summary-should-not-list-calibration
branch
from
8731ccc to
520e1e0
Compare
No description provided.