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

feat: add read_thumbnail #154

Merged
merged 5 commits into from
Sep 12, 2024
Merged

feat: add read_thumbnail #154

merged 5 commits into from
Sep 12, 2024

Conversation

fabien-brulport
Copy link
Contributor

@fabien-brulport fabien-brulport commented Sep 4, 2024

Add a thumbnail method similar to openslide's one

@fabien-brulport fabien-brulport force-pushed the fab/thumbnail branch 3 times, most recently from 03f13b8 to 2dd794e Compare September 4, 2024 15:23
src/view.rs Outdated Show resolved Hide resolved
src/view.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 27 lines in your changes missing coverage. Please review.

Project coverage is 91.02%. Comparing base (bfa4050) to head (d3df632).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/view.rs 46.34% 22 Missing ⚠️
src/bindings.rs 78.94% 4 Missing ⚠️
src/errors.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #154      +/-   ##
===========================================
- Coverage    93.84%   91.02%   -2.83%     
===========================================
  Files            7        8       +1     
  Lines          406      568     +162     
===========================================
+ Hits           381      517     +136     
- Misses          25       51      +26     

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

src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/view.rs Outdated Show resolved Hide resolved
src/bindings.rs Outdated
pub fn new(w: u32, h: u32) -> Self {
Self { w, h }
}
pub fn from_dimensions_range(range: &ffi::DimensionsRange) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry to bother you with this but this function may fail if end < start.
You can impl TryFrom for Size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, If you have suggestions on how to better handle the Err do not hesitate 🙂

src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated
f64::from(dimension_level_0.w) / f64::from(dimension.w),
f64::from(dimension_level_0.h) / f64::from(dimension.h),
);
let level_dowsamples: Vec<f64> = (0..level_count)
Copy link
Owner

Choose a reason for hiding this comment

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

Here is a more Rustish way to achieve that https://github.com/AzHicham/bioformats-rs/blob/714882e1b14a9fb2f4d1c2b5a6a3a6c4f59e7e3d/src/extra/wsi/slide.rs#L98
(0..).map().take_while().enumerate().rfind().unwrap_or()
I’m not sure but it should works ^^

Copy link
Owner

Choose a reason for hiding this comment

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

In biofirmats i think the code is broken we need to check against the first value (max slide dimension ie downsample of 1) then unwraps with the last value :/

Copy link
Contributor Author

@fabien-brulport fabien-brulport Sep 11, 2024

Choose a reason for hiding this comment

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

your implementation looks correct to me !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the one from bioformats

@AzHicham
Copy link
Owner

For the benchmark I’ll merge the PR and fix the workflow later :)

src/bindings.rs Outdated
}
}
impl TryFrom<&ffi::DimensionsRange> for ffi::Size {
type Error = PhilipsSlideError;
Copy link
Owner

Choose a reason for hiding this comment

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

here you can create a new type error
#[derive(Error)]
DimensionsRangeToSizeError{
StepXNull,
StepXNull,
EndXGreaterthanStartX,
EndYGreaterthanStartY,
}

Then add this error into PhilipsSlideError
#[transparent]
DimensionsRangeToSize(#[from] DimensionsRangeToSizeError)

Naming is bad but you get the idea haha

Copy link
Owner

@AzHicham AzHicham left a comment

Choose a reason for hiding this comment

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

Nice GG !!

@AzHicham AzHicham merged commit c929fff into AzHicham:develop Sep 12, 2024
7 of 10 checks passed
@AzHicham AzHicham deleted the fab/thumbnail branch September 12, 2024 12:40
@AzHicham
Copy link
Owner

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants