Skip to content
This repository was archived by the owner on Dec 1, 2025. It is now read-only.

bugfix/pass-series-index-in-biofile-init#401

Merged
evamaxfield merged 30 commits into
mainfrom
bugfix/rename-bioformats-scene-function
May 24, 2022
Merged

bugfix/pass-series-index-in-biofile-init#401
evamaxfield merged 30 commits into
mainfrom
bugfix/rename-bioformats-scene-function

Conversation

@evamaxfield

@evamaxfield evamaxfield commented May 11, 2022

Copy link
Copy Markdown
Collaborator

Description

Somehow slipped through multiple layers of testing and review. The set_scene function in the BioformatsReader was named set_series (which makes sense from the Bioformats API but not consistent with AICSImageIO standard.

Above was incorrect. This PR now actually fixes this. The only problem was that the current scene index of the Reader wasn't being passed down to the BioFile object init during reads.

It also adds a parameter to our multi scene checking function that will check if the data between the two comparison scenes is different or not. By default, this check does not run so as to not make us have to go back and update all of our old tests but potentially useful in the future.

Note: These changes should not be merged until #397 is merged

Pull request recommendations:

  • Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
  • Link to any relevant issue in the PR description. Ex: Resolves [gh-], adds tiff file format support

Resolves #400

  • Provide relevant tests for your feature or bug fix.
  • Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

@evamaxfield evamaxfield added the bug Something isn't working label May 11, 2022
@evamaxfield evamaxfield self-assigned this May 11, 2022
@codecov-commenter

codecov-commenter commented May 11, 2022

Copy link
Copy Markdown

Codecov Report

Merging #401 (cf482e1) into main (ae4fde0) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
- Coverage   94.66%   94.57%   -0.09%     
==========================================
  Files          45       45              
  Lines        3728     3741      +13     
==========================================
+ Hits         3529     3538       +9     
- Misses        199      203       +4     
Impacted Files Coverage Δ
...ts/readers/extra_readers/test_bioformats_reader.py 100.00% <ø> (ø)
aicsimageio/readers/bioformats_reader.py 85.59% <100.00%> (+0.18%) ⬆️
aicsimageio/tests/image_container_test_utils.py 100.00% <100.00%> (ø)
aicsimageio/tests/conftest.py 92.30% <0.00%> (-3.85%) ⬇️
...eio/tests/readers/extra_readers/test_czi_reader.py 98.14% <0.00%> (-1.86%) ⬇️
aicsimageio/readers/ome_tiff_reader.py 98.16% <0.00%> (-0.92%) ⬇️
aicsimageio/readers/czi_reader.py 96.19% <0.00%> (-0.35%) ⬇️
aicsimageio/aics_image.py 88.17% <0.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae4fde0...cf482e1. Read the comment docs.

@AetherUnbound AetherUnbound left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah drat! Nice deprecation warning, this looks good 👍🏼

@evamaxfield evamaxfield changed the title bugfix/rename-bioformats-scene-switch-func bugfix/pass-series-index-in-biofile-init May 19, 2022
@evamaxfield

Copy link
Copy Markdown
Collaborator Author

@tlambert03 @AetherUnbound @toloudis this PR is ready for review! The two bugs were, the BioFile init not being provided the series index and there was a resource-backend-dask-array bug where the open was starting a new file handle / loci object and not setting the scene on open.

@evamaxfield

Copy link
Copy Markdown
Collaborator Author

Also, lol, I have no idea what is going on with credentials @tlambert03 this PR works and your PR doesn't... I wonder if GitHub changed GH Actions secrets access permissions...

@AetherUnbound

Copy link
Copy Markdown
Collaborator

Also, lol, I have no idea what is going on with credentials @tlambert03 this PR works and your PR doesn't... I wonder if GitHub changed GH Actions secrets access permissions...

Oh, I should have realized 😅 If @tlambert03 is submitting a PR from a fork, then the GH actions don't have the same permissions unfortunately. See: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository and https://github.community/t/how-to-use-github-token-for-prs-from-forks/16818

When we encountered this issue, we set up a separate ACCESS_TOKEN in the repositories that would allow the actions to run successfully I believe: WordPress/openverse#199

@AetherUnbound AetherUnbound left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@evamaxfield

Copy link
Copy Markdown
Collaborator Author

Oh, I should have realized sweat_smile If @tlambert03 is submitting a PR from a fork, then the GH actions don't have the same permissions unfortunately. See: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository and https://github.community/t/how-to-use-github-token-for-prs-from-forks/16818

When we encountered this issue, we set up a separate ACCESS_TOKEN in the repositories that would allow the actions to run successfully I believe: WordPress/openverse#199

We already have very permissive action / action secret usage.

image

Plus talley has already PRed to this repo numerous times... 🤷‍♀️ 🤷‍♀️

@AetherUnbound

Copy link
Copy Markdown
Collaborator

Oof, okay yea I'm not sure then 😓

@evamaxfield evamaxfield merged commit 7128bb1 into main May 24, 2022
@evamaxfield evamaxfield deleted the bugfix/rename-bioformats-scene-function branch May 24, 2022 23:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set_scene does not change scene when scene ids are duplicated

4 participants