Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test fixes #564

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

joshua-gould
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.64%. Comparing base (029b6cf) to head (33b2c40).

Current head 33b2c40 differs from pull request most recent head 66e3991

Please upload reports for the commit 66e3991 to get more accurate results.

Files Patch % Lines
aicsimageio/readers/bfio_reader.py 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (029b6cf) and HEAD (33b2c40). Click for more details.

HEAD has 143 uploads less than BASE | Flag | BASE (029b6cf) | HEAD (33b2c40) | |------|------|------| ||155|12|
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #564       +/-   ##
===========================================
- Coverage   86.55%   73.64%   -12.92%     
===========================================
  Files          53       53               
  Lines        4665     4671        +6     
===========================================
- Hits         4038     3440      -598     
- Misses        627     1231      +604     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshua-gould joshua-gould changed the title attempting test fixes test fixes Jun 26, 2024
@joshua-gould
Copy link
Contributor Author

Fixes failing tests in main

Copy link
Collaborator

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! I just want to make sure you are aware of the newer bioio-devs/bioio library.
Anyway it looks like the key fix in here is to pin numpy - is that right? I have a couple other questions in the comments

None,
None,
marks=pytest.mark.xfail(raises=exceptions.UnsupportedFileFormatError),
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

just wondering, why remove these two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading these files with latest libraries does not raise Error

reference.isel(S=i).data == reader.xarray_dask_data.data
).compute()
assert np.all(reference.isel(S=i).data == reader.xarray_data.data)
np.testing.assert_array_equal(reference.isel(S=i).data, reader.xarray_data.data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why remove the dask test too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np.testing.assert_array_equal(reference.isel(S=i).data, reader.xarray_data.data) does what the previous lines were doing

@@ -181,8 +181,7 @@ def set_scene(self, scene_id: Union[str, int]) -> None:
"Scene id: Cannot change scene for "
+ f"{self.__class__.__name__} objects."
)

else:
elif scene_id is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this fixing a real bug in the bfio reader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a bug-just changes behavior when passing a null scene id

@joshua-gould
Copy link
Contributor Author

I wasn't aware of bioio-I will switch to using that library. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants