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

added new parameter and according changes so that limit bounds or oth… #161

Closed
wants to merge 28 commits into from

Conversation

moerlemans
Copy link
Contributor

Offsets can be taken into account when usign the dataset with annotations on mrxs files

Fixes #160

…er offsets can be taken into account when usign the dataset with annotations on mrxs files
@moerlemans moerlemans self-assigned this Aug 1, 2023
@github-actions github-actions bot added the python label Aug 1, 2023
@jonasteuwen
Copy link
Contributor

Can you rebase? Also, the CI/CD should pass

@jonasteuwen
Copy link
Contributor

Make sure CI passes. Run it before you commit, that will also save some CI time.

@jonasteuwen
Copy link
Contributor

I suggest a different way to implement this.

In the WsiAnnotations, you can make a parameter _requires_offset_to_slide_bounds, which you set to True if you read HaloXML. In the read_region you subsequently check if this variable is "True", and if so use the limit_bounds variable.

jonasteuwen and others added 16 commits October 10, 2023 16:57
* Add CITATION.cff file with main contributors
* Add script to bump version in README.md and CITATION.cff
Fix mpp_x <-> mpp_y
* Implement functional interface `rename_labels` for `RenameLabels`
* Improve exception text
…king (#185)

* Fix bug + pass parameters explicitly rather than through kwargs
- Parameter was accidently removed
This fixes the case where geojson annotations are overlayed from small to large area. In that case we didn't properly sort the annotations
@jonasteuwen
Copy link
Contributor

@moerlemans can you rebase once #201 is merged?

@moerlemans moerlemans marked this pull request as draft November 27, 2023 09:11
@moerlemans
Copy link
Contributor Author

The problem is that the limit bounds variable is given by the slide. Which the WsiAnnotation has no access to. So this should be an argument to read region or to the init of wsi annotations. I feel like this last option would be best, initialize it to be zero and use it in read_region. The issue has only appeared for me when using halo xml, but I think it will happen with other types as well probably

…er offsets can be taken into account when usign the dataset with annotations on mrxs files
@github-actions github-actions bot added ci documentation Improvements or additions to documentation labels Nov 27, 2023
@moerlemans moerlemans marked this pull request as ready for review November 27, 2023 15:40
@moerlemans
Copy link
Contributor Author

rebased and made the logic according to the last comment I made.

@jonasteuwen jonasteuwen added this to the v0.4 milestone Dec 1, 2023
@jonasteuwen
Copy link
Contributor

jonasteuwen commented Dec 1, 2023

@moerlemans Can you properly rebase because now you've modified 34 files?

I think you can do something like this:

Add _offset_with_slide_bounds = False to the WsiAnnotations class. Add a property with @property named offset_with_slide_bounds that returns this (so it can only be set within the class itself). Now, you can add this to the constructor of WsiAnnotations, and set it by default to False, and in the Halo constructor, you add this.

Now in the dataset class you an instance check if it is WsiAnnotations or just _offset = slide.get_scaled_slide_bounds(scaling)[0] if getattr(annotations, "offset_with_slide_bounds", False) else (0, 0) and add this to the region.

@jonasteuwen
Copy link
Contributor

I think #204 fixes this

@moerlemans
Copy link
Contributor Author

Indeed this is fixed in #204

@moerlemans moerlemans closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci documentation Improvements or additions to documentation python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotations made by HALO for mrxs slide are not read correctly due to limit bounds
4 participants