Flatten read step - #376
MTakahashi-KWH wants to merge 12 commits into
Conversation
thopkins32
left a comment
thopkins32
left a comment
There was a problem hiding this comment.
The integration test is failing on an assertion. Run with pixi run integration-tests.
You will also have to update the OptimizationLogger as it is directly linked with read_step. It's slightly concerning that the logger tests didn't pick this up...
The docs still mention the array/padding behavior. See src/blop/plan_stubs.py:78-93, docs/source/explanations/callbacks.rst:52-55, and src/blop/callbacks/logger.py:77-79.
| summary.append(f"\n ({outcome_point_count} pts sampled)", style=_DIM_STYLE) | ||
| self._console.print(summary) | ||
|
|
||
| if self._current_step % 5 == 4: |
There was a problem hiding this comment.
Why would you do it this way? self._current_step is incremented before this so summaries are going to be printed at steps 4, 9, 14, ... rather than 5, 10, 15.
| if self._current_step % 5 == 4: | |
| if self._current_step % 5 == 0: |
There was a problem hiding this comment.
Had a mental edge case thought where it would immediately print 0 count stats but it actually preincrements, so ill fix.
| acquire_uid = data.get("acquisition_uid", "") | ||
| # Scalar string comes through as-is; ensure it's a plain string | ||
| if isinstance(acquire_uid, list): | ||
| acquire_uid = acquire_uid[0] if acquire_uid else "" | ||
|
|
||
| n_total = max( | ||
| (len(v) for v in [*param_columns.values(), *outcome_columns.values()]), | ||
| default=1, | ||
| ) | ||
| # Filter out NaN-padded entries: suggestion_ids padded with "" indicate padding | ||
| if suggestion_ids: | ||
| valid_indices = [i for i, sid in enumerate(suggestion_ids) if sid != "" and str(sid).strip() != ""] | ||
| else: | ||
| valid_indices = list(range(n_total)) | ||
| n_valid = len(valid_indices) if valid_indices else n_total | ||
| if isinstance(acquire_uid, Hashable) and acquire_uid not in self._seen_uids: | ||
| self._seen_uids.add(acquire_uid) | ||
| self._current_iteration += 1 |
There was a problem hiding this comment.
We have to figure out something better for acquisition UID or simply exclude it from the callback altogether.
Also, if the acquire_uid is a list we also only use the first element of the list, which is incorrect.
Maybe read_step should be producing an iteration data key in the event document. We can use that for iteration tracking instead of checking for unique acquisition UIDs.
There was a problem hiding this comment.
i was teetering on not adding another iteration inferred variable, given that we could group by uid repr. But, yes, its better explicit
| assert result is doc | ||
| assert console.print.call_count >= 1 | ||
| console.rule.assert_called_once() | ||
| # removed rule assertion as rulered values are now based on number of points rather than per iteration |
There was a problem hiding this comment.
Why keep a comment here?
| # removed rule assertion as rulered values are now based on number of points rather than per iteration |
| logger.event(_make_event(data={"x": 1.5, "y": 3.14})) | ||
|
|
||
| console.rule.assert_called_once_with("Iteration 1", style="blue") | ||
| # removed rule assertion as rulered values are now based on number of points rather than per iteration |
There was a problem hiding this comment.
| # removed rule assertion as rulered values are now based on number of points rather than per iteration |
| def test_event_multiple_iterations(logger, console): | ||
| """Successive events should accumulate without error.""" | ||
| _setup_descriptor(logger) | ||
| logger.event(_make_event(data={"x": 1.0, "y": 10.0})) | ||
| logger.event(_make_event(data={"x": 3.0, "y": 20.0})) | ||
| for _ in range(10): | ||
| logger.event(_make_event(data={"x": 1.0, "y": 10.0})) | ||
| logger.event(_make_event(data={"x": 3.0, "y": 20.0})) | ||
| assert console.print.call_count >= 1 |
There was a problem hiding this comment.
This isn't testing multiple iterations and merely asserting the the call_count >= 1 is doing nothing.
There was a problem hiding this comment.
This is a codecov issue where for some reason previous reports said that the code run every 5 steps was unreached. At least 10 iters was required to cover this, even though the test assertion of rule was called at least twice (top and bottom rule) was passing.
There was a problem hiding this comment.
Remove parameter n_points which is no longer used.
This branch has not been deployed
2 participants
closes #375 also resolving incompatible behavior with upstream libraries