Conversation
|
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python | Mar 27, 2026 4:24p.m. | Review ↗ | |
| Test coverage | Mar 27, 2026 4:24p.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (New Code) | Line Coverage (Overall) |
|---|---|---|
| Aggregate | 100% [✓ above threshold] |
99.9% [▼ down 0.1% from main] |
| Python | 100% [✓ above threshold] |
99.9% [▼ down 0.1% from main] |
➟ Additional coverage metrics may have been reported. See full coverage report ↗
There was a problem hiding this comment.
Pull request overview
Updates CropReporter/PlantExplorer CHL (chlorophyll) import behavior in read_cropreporter to reduce memory usage by storing only the chlorophyll frame as a NumPy array rather than an xarray DataArray, with accompanying documentation updates.
Changes:
- Update CHL processing to extract/store only the Chl frame (dropping Fdark) as a NumPy array on
ps.chlorophyll. - Adjust CHL debug output to write a single-frame image instead of faceted xarray plots.
- Update
read_cropreporterdocumentation to reflect the CHL storage change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plantcv/plantcv/photosynthesis/read_cropreporter.py | Changes CHL import from xarray DataArray cube to a single NumPy frame stored on ps.chlorophyll. |
| docs/photosynthesis_read_cropreporter.md | Updates user-facing documentation to describe CHL output changes (needs terminology/consistency fixes). |
Comments suppressed due to low confidence (1)
plantcv/plantcv/photosynthesis/read_cropreporter.py:24
read_cropreporternow storesps.chlorophyllas a NumPy array (and not exclusively xarray), but the docstring still says the return is "photosynthesis data in xarray DataArray format" and usesPSII_Datainstead of thePSII_dataclass name. Please update the return description/type wording to reflect the mixed xarray/NumPy contents and correct the class naming for accuracy.
Read datacubes from PhenoVation B.V. CropReporter or PlantExplorer cameras into a PSII_Data instance.
Inputs:
filename = .INF filename
Returns:
ps = photosynthesis data in xarray DataArray format
:param filename: str
:return ps: plantcv.plantcv.classes.PSII_data
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…er/plantcv into 1862-xarray-imports
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…er/plantcv into 1862-xarray-imports
joshqsumner
left a comment
There was a problem hiding this comment.
I think this is good per the rest of the cropreporter stuff, it works for me locally and seems clean.
I took a cursory stab at monkeypatching the tests and it wasn't super forthcoming with how many unexported functions are used in the main readers. Goal was to reduce the amount of stuff in the testdata that only pertains to these cropreporter functions since a 12Mb dataset felt very large.
Looking more broadly, I don't think adding this new test data is unreasonable but it may be worth some effort to clean all the cropreporter tests up to use fewer large files. I am not sure which of these could be combined if any, but there are some large files in here and some are duplicated just different by 1 character in the filename.
(plantcv) josh@Precision-7550:~/plantcv$ find tests/testdata -type f -exec du -ah {} + | grep -v "/$" | sort -rh | head -15
53M tests/testdata/cropreporter_pmt/PMT_E0001P0007N0001_GCU24100090_20260226.DAT
51M tests/testdata/cropreporter_v441/PSII_PSL_020321_WT_TOP_1.DAT
51M tests/testdata/cropreporter_v441/PSII_PSD_020321_WT_TOP_1.DAT
42M tests/testdata/cropreporter_v653/PSL_dark_light.DAT
42M tests/testdata/cropreporter_v653/PSD_dark_light.DAT
24M tests/testdata/cropreporter_v653/PML_dark_light.DAT
24M tests/testdata/cropreporter_v653/PMD_dark_light.DAT
18M tests/testdata/cropreporter_v653/CLR_dark_light.DAT
18M tests/testdata/cropreporter_gfp/GFP_DYSeed_20251222191634684.DAT
15M tests/testdata/cropreporter_npq/PSII_NPQ_020321_WT_TOP_1.DAT
12M tests/testdata/cropreporter_v653/CHL_dark_light.DAT
12M tests/testdata/cropreporter_rfp/RFP_DYSeed_20251222191634684.DAT
12M tests/testdata/cropreporter_chl/CHL_2025-12-12_tob1-ojip_20251212213103380.DAT
12M tests/testdata/cropreporter_aph/APH_2025-12-12_tob1_20251212205712029.DAT
9.7M tests/testdata/colorcard_img.png
|
Forgot to push local changes before submitting approval, just stylistic some docstring edits |
joshqsumner
left a comment
There was a problem hiding this comment.
@nfahlgren per dev meeting conversation I removed the additional test data and used the chl data from v441.
Describe your changes
Updating the
read_cropreporter.pyfunction so that CHL (chlorophyll) frames are imported as numpy instead of xarray. This will save memory.Type of update
Is this a:
Associated issues
Associated with Issue 1862
For the reviewer
See this page for instructions on how to review the pull request.
plantcv/mkdocs.ymlupdating.md