search

SBU-BMI / wsinfer

🔥 🚀 Blazingly fast pipeline for patch-based classification in whole slide images
https://wsinfer.readthedocs.io
Apache License 2.0
55 stars 9 forks source link

Fix/geojson command #191

Closed swaradgat19 closed 10 months ago

swaradgat19 commented 10 months ago

fixes #181

swaradgat19 commented 10 months ago

@kaczmarj So I made a separate branch and opened a PR (Including the fix #188 ). The pytorch test has passed, but the docker and the ubuntu tests haven't

kaczmarj commented 10 months ago

thanks @swaradgat19 -

flake8 is failing with wsinfer/write_geojson.py:92:89: E501 line too long (89 > 88 characters) . seems that one line is just slightly too long :) you can run black on the file and it should fix it. so run black wsinfer/ and then flake8 wsinfer and see if that helps.

i re-queued the failing tests -- maybe it was something random. i could not find any useful error messages in the ubuntu and docker tests.

kaczmarj commented 10 months ago

the docker and ubuntu tests are still failing. i will have to run those locally to see what is going on.

kaczmarj commented 10 months ago

@swaradgat19 - i think i figured out the failure. in

https://github.com/SBU-BMI/wsinfer/blob/59c0f5c1fe44baab0a2e8616a8075ef238259bdd/.github/workflows/ci.yml#L107-L108

update the directory to use model-outputs-csv

same in https://github.com/SBU-BMI/wsinfer/blob/59c0f5c1fe44baab0a2e8616a8075ef238259bdd/.github/workflows/ci.yml#L121C1-L121C80

i am not sure why the windows tests are passing.... the directory seems incorrect. either i didn't write that test properly or the model-outputs directory is still being made somehow.

swaradgat19 commented 10 months ago

Oh sure. I'll make the changes

kaczmarj commented 10 months ago

nice, the ubuntu test passes now!

flake8 is throwing the following error

wsinfer/write_geojson.py:92:89: E501 line too long (89 > 88 characters)
swaradgat19 commented 10 months ago

I ran flake8 wsinfer, but it just gives me a bunch of errors saying the lines are long in multiple files. I'll shorten all and put in a commit.

Also, since we're generating model-outputs-geojson, should we include that test in the yml as well?

kaczmarj commented 10 months ago

I ran flake8 wsinfer, but it just gives me a bunch of errors saying the lines are long in multiple files. I'll shorten all and put in a commit.

i think you can shorten line 92 of wsinfer/write_geojson.py and we should be good.

yes we should add model-outputs-geojson. we don't necessarily have to test the contents now, but let's definitely test that the expected files are made.

in a future PR, we can add tests for the geojson conversion.

kaczmarj commented 10 months ago

excellent, all the tests passed! let me try this out on my machine to see how thing are. then i will merge. thanks so much @swaradgat19 !

swaradgat19 commented 10 months ago

My pleasure! :) @kaczmarj