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

Optimized Area thresholding #983

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AuroVarat
Copy link

This modification enhances the area thresholding function, significantly improving processing speed, especially for large images.

@ns-rse
Copy link
Collaborator

ns-rse commented Oct 25, 2024

Thanks for the Pull Request @AuroVarat I'll take a look through this on Monday.

In the mean time would you be able to address the failing tests please?

If you're not familiar with tests the software development section of our docs has some advice. You'll want to pip install -e .[dev,tests.docs] and ideally install the pre-commit hooks with pre-commit install. We use the pytest-testmon extension and so you'd have to run the tests once to check everything is working with pytest --testmon and then when you make commits only the tests that are affected will run.

Note also that pre-commit.ci has modified this branch by fixing some minor linting errors so you may want to git pull to get those changes (although if you install pre-commit locally it will make them before you can make your next commit again).

@AuroVarat
Copy link
Author

Hi, I am not familiar with tests but I managed to go through the steps and do the "pytest --testmon". It fails 8 tests, but it fails 8 tests regardless of my changes ( I did the same test by running it on the current main branch). Maybe I am doing something wrong, I will look at it when I have more time.

But if you see my changes, they are minimal and should not result in this failure. The code seems to be working fine when I am doing my analysis.

Thanks.

@AuroVarat
Copy link
Author

AuroVarat commented Jan 20, 2025

@ns-rse Hi Neil, Hope you are well. Were you able to merge it? If not, I can look into the test failures again.

@ns-rse
Copy link
Collaborator

ns-rse commented Jan 20, 2025

Hi @AuroVarat

Apologies I've not had time to investigate why the tests are failing as I'm not full time on this project and have had various other tasks to work through.

The logs show that there are two elements that are different in the revised code. How big an issue that might be isn't something I can really answer but we have tests in place to ensure that changes under the hood don't change the expected results. You could get more information on where these have changed running the specific test with...

pytest -vv tests/test_grains.py::test_find_grains

...although there may be other places where tests fail as we use the -x flag which exits the test suite on the first failure that is encountered so it would be worth running the whole test suite (this can be done in parallel if you install all of the dev optional dependencies in your virtual environment with pytest -n # where # is the number of cores to use which will depend on your system and what else you want to do whilst its running).

If you are new to testing you may find the following tips on Contributing useful as they show how to get setup with a development environment and install the pre-commit hooks that we have in place which performs various linting.

I've got some failing tests of my own to work through but feel free to message if you've any questions.

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.

2 participants