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

Update imagewriter.py with tile and coordinate checks and corrections #65

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

JoeySpronck
Copy link
Collaborator

As discussed with multiple people from our group the wrapped ASAP image writer sometimes exhibits unexaplainable behaviour that is very hard to debug. In this PR I intend to prevent some of the obvious mistakes to pass without errors. If possible I correct the tile. By doing this the writer becomes more robust and guides the user on what to fix, instead of ambiguously trying to keep the loop going without throwing warnings or errors.

I think the _crop_tile function and the _get_row_col function already partly do some of these checks, but neither throw errors or warnings, causign outputs that are incorrect and leaving users unaware of what went wrong. I left them in right now though, the new function takes care of possible errors.

I havent tested this yet but I would like to share this already so we can finalize it together.
@martvanrijthoven @daangeijs @carlijnlems @nfsuysal @leandervaneekelen @rolandnemeth000

@coveralls
Copy link

coveralls commented Aug 30, 2024

Pull Request Test Coverage Report for Build 11382794478

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 12 of 18 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 72.629%

Changes Missing Coverage Covered Lines Changed/Added Lines %
wholeslidedata/interoperability/asap/imagewriter.py 12 18 66.67%
Totals Coverage Status
Change from base Build 10526088141: -0.04%
Covered Lines: 2221
Relevant Lines: 3058

💛 - Coveralls

… mask wasnt needed. As long as the tile size is correct this tile will be written correctly
@JoeySpronck
Copy link
Collaborator Author

Hey @martvanrijthoven, I adjusted the PR. One check/adjustment wasnt needed, I removed it. All proposed check now are pretty much straightforward and will prevent ambiguous/incorrect behaviour. Also guides the user in adjusting errors.

@martvanrijthoven
Copy link
Collaborator

Hey Joey,

Super nice looks good, would you say its ready for merge?

One small note, I am not a big fan of the (my opinion: redundant) comments above already self explanatory code you wrote. But up to you to remove or keep them.

@JoeySpronck
Copy link
Collaborator Author

@martvanrijthoven Agreed about the comments, removed them, ready for merge!

@martvanrijthoven martvanrijthoven merged commit 264dada into DIAGNijmegen:main Oct 17, 2024
2 of 3 checks passed
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