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

293 add smoothingfilter cli functions #307

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

msorvoja
Copy link
Collaborator

Adds cli functions for the filter tools. There is a problem with Lee enhanced, frost and kuan filters which I was unable to solve, hence the draft PR. Could you @msmiyels look into it as you have implemented these tools? Those three filter function all give SystemError: <built-in function len> returned a result with an exception set with the same data (small_raster.tif in tests/data/remote) as the tests for these tools use.

@msorvoja msorvoja added preprocessing Feature related to preprocessing functions CLI Command-line interface related labels Jan 31, 2024
@msorvoja msorvoja linked an issue Jan 31, 2024 that may be closed by this pull request
@msmiyels
Copy link
Collaborator

msmiyels commented Feb 1, 2024

Servus Mika,

thank you for integrating this into the CLI. I never came across this type of error 🙄 and to be honest, currently have no capacity to dive in.

I would probably need to test this out by myself, so is there a doc containing info how to test the CLL implementation or could you provide me a short how to for replicating this on my end?

Take your time on this one, since I assume that I cannot check it out prior to week 8. So if you have other functions to implement, I recommend to focus on these and do not wait for my reply the next couple of days.

Just for clarifiying my understanding, the Conda fails in the PR:

ERROR tests/checks/check_raster_grids_test.py
ERROR tests/conversions/csv_to_geodataframe_test.py
ERROR tests/conversions/raster_to_dataframe_test.py

refer to other files/functions than the filters. That's probably not the issue, right?

Cheers,

Michael

@msorvoja
Copy link
Collaborator Author

msorvoja commented Feb 8, 2024

Hello Michael,

Thank you for looking into this! I'm not sure how familiar you are with these CLI functions so here are (hopefully sufficient) instructions to get you started:

You need a virtual environment into which you install eis_toolkit. Inside container,

poetry build

builds the .whl file inside dist folder. Activate the venv if not already and install the wheel

pip install eis_toolkit-0.1.0-py3-none-any.whl --force-reinstall

Note, that every time you create a new CLI function or make any modifications to the existing ones, you need to rebuild the wheel and install it.

Now with

eis --help

you should be able to see all the cli functions available. To get help with any of the CLI functions, just type

eis function-name-here-cli --help

To test, e.g., the Frost filter with the default parameters, type

eis frost-filter-cli --input-raster small_raster.tif --output-raster test.tif

This should give you the same error as it gives to me.

The Conda fails shouldn't be related to this as they seem to happen in other PRs as well. @nmaarnio is looking into it.

-Mika

@nmaarnio
Copy link
Collaborator

Hi,

If you are using Poetry (or Docker) environment, you don't actually need to create any additional venv. Just run

poetry run eis frost-filter-cli --input-raster small_raster.tif --output-raster test.tif etc.

I tried to solve this quickly, but with no success. I could run the notebook without any issues, but the CLI functions give the same error Mika pointed out. It would still probably be worth test this with different Scipy version, although it puzzles me that the functions run in notebooks/scripts with the current version...

@msmiyels
Copy link
Collaborator

Ahoi, I‘m currently on my way back from traveling and will check that out asap. But I‘ll probably be only 1-2 days in the office this week.

Cheers,
Michael

Bug fixes for kuan, frost and lee-enhanced speckle filters.
Functions had inconsistent type declarations for n_looks (must be integer) and damping_factor (must be Number or float). That issue apparently confused the CLI calls.

Also updated the cli.py to be consistent with the speckle code.
@msmiyels
Copy link
Collaborator

msmiyels commented Mar 1, 2024

Ahoi @nmaarnio, @msorvoja,

checked this one out today. There were some minor inconsistencies in the respective call- and private function definitions ✍️ , which obviously caused these errors 🛑 for the kuan, frost and lee-enhanced filters. Also updated the CLI file to be consistent with the changes. Please check that out on your machines 🧪 and give it a go if okay 🚀

Cheers and a nice weekend 😄
Michael

Copy link
Collaborator

@nmaarnio nmaarnio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Micha! Worked for me, so let's merge these now.

I removed the debug prints and TODO comments Mika had put there before I merge.

@nmaarnio nmaarnio marked this pull request as ready for review March 4, 2024 07:32
@nmaarnio nmaarnio merged commit 5813e55 into master Mar 4, 2024
6 checks passed
@nmaarnio nmaarnio deleted the 293-add-smoothingfilter-cli-functions branch March 4, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Command-line interface related preprocessing Feature related to preprocessing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add smoothing/filter CLI functions
3 participants