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

Fix/geojson command #191

Merged
merged 19 commits into from
Sep 20, 2023
Merged

Fix/geojson command #191

merged 19 commits into from
Sep 20, 2023

Conversation

swaradgat19
Copy link
Contributor

@swaradgat19 swaradgat19 commented Sep 16, 2023

fixes #181

@swaradgat19
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Member

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

@kaczmarj
Copy link
Member

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

test -f results/model-outputs/JP2K-33003-1.csv
test $(wc -l < results/model-outputs/JP2K-33003-1.csv) -eq 675

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
Copy link
Contributor Author

Oh sure. I'll make the changes

@kaczmarj
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

My pleasure! :) @kaczmarj

@kaczmarj kaczmarj merged commit c41a9b8 into SBU-BMI:main Sep 20, 2023
11 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.

always create geojson outputs with 'wsinfer run'
2 participants