Conversation
yfukai
requested review from
BrianWhitneyAI,
SeanDuHare,
aswallace,
kmitcham,
pgarrison and
toloudis
|
Cc: @BrianWhitneyAI |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 86.97% 91.05% +4.07%
==========================================
Files 6 16 +10
Lines 453 1006 +553
==========================================
+ Hits 394 916 +522
- Misses 59 90 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| czi = CziFile(open_resource.f) | ||
| return frame_acquisition_times( | ||
| czi=czi, | ||
| current_scene=self.current_scene_index, |
There was a problem hiding this comment.
Just double checking here, does the index always correlate to the correct scene identification? I remember some issues in different mappings.
There was a problem hiding this comment.
I believe so, since similar logic is used in another place. (In both cases, eventually get_coords_from_kwargs is called.)
| ) | ||
| if acquisition_time is None: | ||
| continue | ||
| d = {**subblock_info, "acquisition_time": acquisition_time} |
There was a problem hiding this comment.
This format definitely needs to be documented, but that is less important in the current draft.
There was a problem hiding this comment.
Right. This could be transformed into xarray, but not really straightforward without pandas.
| Reader(uri, use_aicspylibczi=True) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
I think the test suite needs some work (Understandably less important at this stage). I think that this current test is overly complex for the type of unit testing we would expect. I think that we could have two tests here.
-
A test similar to this one but it makes direct assertions about the output acq time. This should be a test param for readability. I think we do something similar in our other standard metadata tests. We should include at least one param here that is none.
-
A test asserting the difference in values across scene selection
There was a problem hiding this comment.
Hi, thanks for your comment, I updated the tests!
yfukai
left a comment
yfukai
left a comment
There was a problem hiding this comment.
Hi, thanks for your comment @BrianWhitneyAI, I've updated the code.
We still need to decide on the returning data (list of dict or xarray).
|
Hi @BrianWhitneyAI, could I have your comment? Also, do you have any opinion on the return type? |
3 participants
Link to Relevant Issue
This pull request (partly) resolves bioio-devs/bioio#172
Description of Changes
This is a draft implementation for
frame_acquisition_times. Currently, it returns a list of dict asfor the selected scene, but inputs are welcome! (Maybe we should pop "S" from the dict.)