-
-
Notifications
You must be signed in to change notification settings - Fork 62
add conversion level - magnification factor, when possible #319
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per comments
Codecov Report
@@ Coverage Diff @@
## master #319 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 1294 1307 +13
Branches 124 127 +3
=========================================
+ Hits 1294 1307 +13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the tests that are included are ok.
However, since the integration test it_knows_its_magnification_factors
is basically run only on CI, I would add:
- a test case for that method to use the local
cmu-1-small-region.svs
- two unit tests using a fake svs and mocking
properties
property to include or not"openslide.objective-power"
and"f"openslide.level[{level}].downsample"
to better control the flow. Let's try to have 100% coverage on the new modifications without the help of the integration tests
f056e68
to
9a2c72d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5e04da8
to
5056046
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but let's see if you can address the minor comment I just wrote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM well done 👯
Address Issue #294