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

Improve feature wrapper and fractal task #61

Merged
merged 11 commits into from
Apr 19, 2023
Merged

Conversation

jluethi
Copy link
Contributor

@jluethi jluethi commented Apr 18, 2023

I was anyway working on integrating the feature_wrapper function better into the Fractal task, so I also did a small refactor to address #59.

The new get_regionprops_measurements is simplified and calls 3 helper functions for the main measurements. In my tests, it still performs the same as before (except the C01.x_pos_weighted_pix etc. measurement being quite different, but that was an intentional bug fix, right?

Fractal testing is still ongoing, but this can already be merged now to avoid to many different branches in parallel.

@jluethi
Copy link
Contributor Author

jluethi commented Apr 18, 2023

@enricotagliavini Let me know if you're happy with this cleanup approach :)

@enricotagliavini
Copy link
Contributor

It's a good start, the logic is more clear and, in general, it's more readable. I might have some further ideas, let's also see what @nrepina thinks.

@nrepina
Copy link
Collaborator

nrepina commented Apr 19, 2023

Very nice, thank you @jluethi, and perfect timing for the refactor! I tested and also see no difference in output compared to feature_wrapper prior to refactor.

Indeed, the change in weighted z centroid was an intentional bug fix.

I will approve the merge and let's chat @enricotagliavini if we want to improve further.

@nrepina nrepina merged commit a93f221 into main Apr 19, 2023
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.

Cleanup the get_regionprops_measurements() function in feature_wrapper.py
3 participants