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

Generalize photosynthesis filenaming #984

Conversation

wurDevTim
Copy link
Collaborator

@wurDevTim wurDevTim commented Nov 29, 2022

Describe your changes
Phenovation changed the naming convention over time. Some small changes where made to add support for the new files.
The new code is tested with data from both new and old cropreporters.

Type of update
Is this an update of the cropreporter filenaming to make the OJIP analysis work with old and new cropreporters.

Associated issues
#978

Additional context
The current git only contains files which uses the 'old' metadata format.
I have some test images with the new format, who can send these to? Or where to upload?
Maybe also add a test for these new files?

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

@HaleySchuhl HaleySchuhl self-requested a review November 29, 2022 17:08
@HaleySchuhl HaleySchuhl added enhancement Enhancements to existing features update Updates an existing feature/method labels Nov 29, 2022
@HaleySchuhl HaleySchuhl added this to Pull Requests in PlantCV4 via automation Nov 29, 2022
@HaleySchuhl HaleySchuhl added this to the PlantCV v4.x milestone Nov 29, 2022
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #984 (484a55d) into release-4.0 (f6c6937) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-4.0      #984   +/-   ##
=============================================
  Coverage       100.00%   100.00%           
=============================================
  Files              171       171           
  Lines             7528      7540   +12     
=============================================
+ Hits              7528      7540   +12     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lantcv/plantcv/photosynthesis/read_cropreporter.py 100.00% <100.00%> (ø)

Copy link
Contributor

@HaleySchuhl HaleySchuhl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wurDevTim thanks for opening this PR. The code looks good, but the additional functionality at L78 - L80 and L121-L123 are not getting hit by the new test. Also a clarification sentence added to the documentation for photosynthesis_read_cropreporter would be good too!

@wurDevTim
Copy link
Collaborator Author

@HaleySchuhl, I see, to hit those lines I have to add testdata from our cropreporter (the new file format).
The current ones are called PSII_PSD_supopt_temp_btx623_22_rep1.DAT and PSII_PSD_supopt_temp_btx623_22_rep1.INF.
Is there a naming preference for the new files? To make it easy to keep the two version apart or can I use any name I like?

@HaleySchuhl
Copy link
Contributor

@wurDevTim pretty much any name you'd like!

@wurDevTim
Copy link
Collaborator Author

@HaleySchuhl Added the files by making subfolders for the different cropreporter versions. Also updated the docs, let me know if there are any other things you like me to change.

@wurDevTim
Copy link
Collaborator Author

After quite some mails back and forth to phenovation we figured out that when 'SaveAllFrames' is set to '0' it only returns 3 frames: Fdark, F0, Fm for the PSD and Flight, Fp, Fmp for the PSL.

However, the framelabels are not updated. therefore the code below tries to set frame labels to frames which don't exist:

elif 'FqFmFrameFsp' in metadata:
   fsp_frame = int(metadata["FqFmFrameFsp"]) + 1
   fmp_frame = int(metadata["FqFmFrameFmp"]) + 1            
elif 'LtOjipFrameFsp' in metadata:
   fsp_frame = int(metadata["LtOjipFrameFsp"]) + 1
   fmp_frame = int(metadata["LtOjipFrameFmp"]) + 1

To make plantcv robust against this bug I changed the code to:

if metadata['SaveAllFrames'] == '0':
   fsp_frame = 1
   fmp_frame = 2
elif 'FqFmFrameFsp' in metadata:
   fsp_frame = int(metadata["FqFmFrameFsp"]) + 1
   fmp_frame = int(metadata["FqFmFrameFmp"]) + 1            
elif 'LtOjipFrameFsp' in metadata:
   fsp_frame = int(metadata["LtOjipFrameFsp"]) + 1
   fmp_frame = int(metadata["LtOjipFrameFmp"]) + 1

Copy link
Member

@nfahlgren nfahlgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wurDevTim, sorry for the long delay. I think this looks great, I just have a few suggestions for simplifying the code a bit

plantcv/plantcv/photosynthesis/read_cropreporter.py Outdated Show resolved Hide resolved
plantcv/plantcv/photosynthesis/read_cropreporter.py Outdated Show resolved Hide resolved
plantcv/plantcv/photosynthesis/read_cropreporter.py Outdated Show resolved Hide resolved
@nfahlgren nfahlgren modified the milestones: PlantCV v4.x, PlantCV v4.0 Jun 17, 2023
@nfahlgren nfahlgren changed the base branch from 4.x to release-4.0 June 17, 2023 16:02
@nfahlgren nfahlgren dismissed HaleySchuhl’s stale review June 17, 2023 16:28

Tests were updated to achieve 100% coverage

@nfahlgren nfahlgren merged commit 3eb9ffe into danforthcenter:release-4.0 Jun 17, 2023
5 of 6 checks passed
PlantCV4 automation moved this from Pull Requests to Done Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancements to existing features ready to review update Updates an existing feature/method
Projects
PlantCV4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants