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
New GLCM properties to graycoprops #7375
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @SerodioJ for the new feature suggestion! Even though we usually suggest opening an issue first to suggest new features (or did I miss that), this is looking very good for a first contribution.
I haven't looked into this too deeply yet, but it seems like this is something we can add to scikit-image. Though, I am curious about the context around this PR. How did you come across this and why do you think this would be useful to add to the library?
I've also left a few minor suggestions from a first review.
Thanks for the review @lagru. Regarding the context of this PR. I am currently working with seismic processing, and one of the things I am using are attributes based on the GLCM. As I am using Python on my experiments, I ended up using scikit-image to compute some attributes, but not all of the ones listed here where available on the library (the ones added by this PR). Also thank you for the suggestions. I will update the PR with them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the suggestions. The implementation looks good. Though, I'd like to make sure that we are testing the correct things. Especially with regards to -np.log(P, where=(P != 0), ...)
.
@lagru what do you think would be the best approach to test the properties?
|
Hi @lagru, is there anything that must be updated on the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Sorry for the delayed feedback. Would you mind adding the clarification on where the reference values come from?
Oh, feel free to ignore the failures on MacOS for now. We need to fix that, hopefully soon. |
I was so confused for a minute, why this PR-branch is passing on MacOS even though you didn't merge the fixes from #7414. I think it's because GitHub runs the job on the would-be merge between this branch and the target? So the fixes are included in the job just by being present on Thanks for the update though. |
Hi @lagru. When will it be merged? And also whats is the frequency of new releases of the package? |
@SerodioJ, hard to say. It depends on when a second pair of maintainer eyes has the free time & priority to review this. Could be tomorrow could be much later. Sorry, open source contributions can sometimes require a bit of time due to the volunteer nature of most parties involved. I've added the PR manually and will request reviews, though since this should be ready if not close. |
Description
This PR extends the graycoprops function with new properties that can be computed from the GLCM. The new properties are:
These properties can be found as texture attributes in the OpendTect software, used in seismic processing.
Checklist
./doc/examples
for new featuresRelease note
We use changelist to
compile each pull request into an item of the release notes. Please refer to
the instructions
and past release notes
for guidance and examples.