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

Added NPCI and GLI indices to spectral_index #1593

Merged
merged 19 commits into from
Sep 17, 2024

Conversation

rayn-alex
Copy link
Collaborator

@rayn-alex rayn-alex commented Sep 3, 2024

Describe your changes
I added two additional indices, Normalized Pigment Chlorophyll Index and Green Leave Index, to spectral_index. Added also the respective tests and documentation (including references).

Type of update
Is this a:

  • New feature or feature enhancement
  • Update to documentation

Associated issues
None

Additional context
None

For the reviewer
See this page for instructions on how to review the pull request.

  • PR functionality reviewed in a Jupyter Notebook
  • All tests pass
  • Test coverage remains 100%
  • Documentation tested
  • New documentation pages added to plantcv/mkdocs.yml
  • Changes to function input/output signatures added to updating.md
  • Code reviewed
  • PR approved

Copy link

deepsource-io bot commented Sep 3, 2024

Here's the code health analysis summary for commits 1242505..4b08905. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Python LogoPython✅ SuccessView Check ↗
DeepSource Test coverage LogoTest coverage⚠️ Artifact not reportedTimed out: Artifact was never reportedView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@HaleySchuhl HaleySchuhl self-requested a review September 6, 2024 16:04
@HaleySchuhl
Copy link
Contributor

Thanks @rayn-alex for adding this! I'll plan to review it early next week.

I have also asked @nfahlgren to add you as a contributor so that you can branch the main plantcv repo in any future PRs (DeepSource is not reporting code coverage since it's branched from your repo). Hope you have a great rest of your Friday.

@rayn-alex
Copy link
Collaborator Author

Great, thank you and good to know! Already received the invitation.

Have a pleasant week ahead!

@HaleySchuhl
Copy link
Contributor

Good morning @rayn-alex , thanks again for contributing.

I've started on reviewing this PR and the NPCI functionality & docs look great.

I noticed GLI can be calculated from the bands available in an RGB image, so to match the patter in other indices like this (e.g. EGI) I believe we should update the input parameter from hsi to rgb_img. While testing each new index on the small maize HSI that's used in tests, I noticed the resulting GLI index has a max value of 183 although the predicted data range for the index is [-1,1]. This prompted me to check the publication of the GLI index and they specify the input bands ought to be [0,255], which is true of the pseudo_rgb. I tested this with the code below.

%matplotlib widget
import os
import cv2
import matplotlib
import numpy as np
from plantcv import plantcv as pcv

pcv.params.debug = "plot"
test_img = "../../GitHub/plantcv/tests/testdata/corn-kernel-hyperspectral.raw"
# Read in the hyperspectral data
array = pcv.readimage(filename=test_img, mode="envi")

rgb_img = array.pseudo_rgb

blue, green, red = cv2.split(rgb_img)
# Calculate GLI 
## GLI = (2 * GREEN - RED - BLUE) / (2 * GREEN + RED + BLUE)
with np.errstate(divide="ignore", invalid="ignore"):
    r = red.astype(np.float32)
    g = green.astype(np.float32)
    b = blue.astype(np.float32)
    index_array_raw = (2 * g - r - b) / (2 * g + r + b)

print(np.max(index_array_raw))
print(np.min(index_array_raw))

If it's ok with you, I can make these changes today, and then have you test them locally before Noah merges the changes into main?

@rayn-alex
Copy link
Collaborator Author

Hey @HaleySchuhl,

I added the GLI index as hsi index mainly because I'm currently only working with hyperspectral images and going the through pseudo_rgb would change the compatibility.

I absolutely see the point and I think it should be changed as you suggested it.

However, would it make sense to add support for both input types, rgb_img and hsi? It's probably just a simple input check in the beginning. I could take care of it. I think it would only affect EGI and the GLI index so far.

What do you think?

@HaleySchuhl
Copy link
Contributor

@rayn-alex I really like the idea of added flexibility to accept both HSI and RGB in those two indices. Very in favor of detecting and handling expected datatypes.

The one consideration is that changing the input parameter in EGI to something more general like img would be a breaking change, and we're trying not to introduce any of those between major releases. So can you go ahead and add the check to both functions and allow them to handle both input datatypes, but preserve the rgb_img input name in EGI?

@rayn-alex
Copy link
Collaborator Author

@HaleySchuhl
Great! Sure, I can preserve the rgb_img in EGI. I'll work on this in the next days.

@HaleySchuhl HaleySchuhl added new feature New feature ideas and solutions work in progress Mark work in progress update Updates an existing feature/method labels Sep 11, 2024
@rayn-alex
Copy link
Collaborator Author

@HaleySchuhl Done!

I added new tests and updated the docs as well. I used different wavelength presets for the EGI RGB wavelength and also increased the default distance to 40nm because the original publication only uses the RGB values.

~R700 for RED, ~R530 for GREEN, and ~R460 for BLUE

@HaleySchuhl HaleySchuhl added ready to review and removed work in progress Mark work in progress labels Sep 16, 2024
@HaleySchuhl
Copy link
Contributor

HaleySchuhl commented Sep 16, 2024

Happy Monday @rayn-alex , (edited)

I'm reviewing the updated functionality this morning and pushed a few minor changes so far related to DeepSource complaints.

While testing the functions locally, I ran into similar inconsistent-data-range outputs for the GLI. The RGB input functionality works nicely and gives an index with values all within the predicted range [-1,1]. However, the Spectral_data case still is not converting the datatype of the reflectance bands prior to calculating the index, so the calculated GLI on the unit test image still has values outside the predicted range.

Converting to float32 in both cases, and then calculating the index outside the if logic ought to resolve this. I could take care of it and then ping Noah to do another quick review before he merges this into main 👍 thanks again for collaborating with us!

@HaleySchuhl HaleySchuhl added this to the PlantCV v4.5 milestone Sep 16, 2024
@nfahlgren nfahlgren merged commit 0d60cc6 into danforthcenter:main Sep 17, 2024
4 of 5 checks passed
@nfahlgren nfahlgren modified the milestones: PlantCV v4.5, PlantCV v4.6 Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature ideas and solutions ready to review update Updates an existing feature/method
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants