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

Cropreporter adding pam #1217

Merged
merged 12 commits into from
Aug 10, 2023
Merged

Conversation

JTvD
Copy link
Collaborator

@JTvD JTvD commented Jul 6, 2023

As mentioned in #978 we first wanted to add OJIP and than follow with other protocols. At the moment a lot of researchers are using PAM, there where some issues with the format. Luckily a colleague at the WUR managed to figure this out. I will double check with colleagues but for as far as I know the PSII efficiency calculation are the same for OJIP and PAM.

The adaptation takes quite some time, therefor we quickly collected this test data by running the different protocols after each other. I will request a measurement day to collect data from a real plant.

There are a lot of big changes in the 'release-4.0' branch which take some time to process, please let me know if I missed something.

Type of update
New feature or feature enhancement

Associated issues
Reference associated issue numbers. Does this pull request close any issues?

Additional context
Upgrading the photosynthesis library to support the new capabilities used at WUR.

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

@JTvD JTvD self-assigned this Jul 6, 2023
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #1217 (f0f2bbc) into release-4.0 (95dffb5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-4.0     #1217   +/-   ##
=============================================
  Coverage       100.00%   100.00%           
=============================================
  Files              157       157           
  Lines             6757      6777   +20     
=============================================
+ Hits              6757      6777   +20     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

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

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

@nfahlgren
Copy link
Member

Hi @wurDevTim, thanks for the further updates! We have been doing a little work to understand the differences between the measurement protocols just so that we can make it clear in a tutorial what they all mean.

One question directly about this pull request. The function pcv.visualize.chlorophyll_fluorescence was designed to plot the fluorescence induction curves produced by the OJIP method. Since the PAM method does not produce the same curve, should we have the function recognize the PAM datasets still? If no, is a different plot useful? @kmurphy61 and I were discussing adding an additional time value frame label to the data structure so that we could potentially plot something like this: https://www.researchgate.net/profile/Alexander-Ruban-2/publication/319123856/figure/fig2/AS:631664216653865@1527611893716/Typical-PAM-fluorescence-measurement-of-an-Arabidopsis-leaf-showing-induction-and_W640.jpg

Some other things we discussed, that could probably be a separate pull request after merging this, are:

  • Renaming the datasets because darkadapted, etc. are no longer specific. We could instead have something like ojip_dark, ojip_light, pam_dark, pam_light.
  • Are the PAM data compatible with measuring NPQ? We think not based on the documentation but are not 100% sure.

@nfahlgren
Copy link
Member

@all-contributors please add @wurDevTim for code, doc, test.

@allcontributors
Copy link
Contributor

@nfahlgren

I've put up a pull request to add @wurDevTim! 🎉

@JTvD
Copy link
Collaborator Author

JTvD commented Jul 18, 2023

Heey @nfahlgren, due to vacations things are moving a bit slower these months.
Regarding plots, the fluorescence induction curves are most interesting for the OJIP protocol when all frames are saved. If we don’t save all frames or do a PAM measurement we get a fixed number of frames and this graph does not say much. The graph you suggest could be nice but I don’t think we can get enough information from the cropreporter to create it. Here I have to admit that I am no photosynthesis expert, luckily some of my colleagues are. Usually they don’t even look at the fluorescence, instead they directly compute the NPQ and PSI2. Therefore I doubt if it is worth changing now.

You might have noticed that the fluorescence and photosynthesis look terrible on the test data, this is due to the lack of adaption on the images of this leaf. Usually we don’t run all protocols on the same plant, but there is data coming. When that is in we plan to dive into the NPQ & PSI2 calculations.
If you want a more insight in how and what is typically done at the WUR I can try to setup a meeting? For the more technical question we have the option to call in support from phenovation.

Lastly, if the renaming does not break backwards compatibility it would be nice, I can pick that up next.

@nfahlgren nfahlgren mentioned this pull request Jul 26, 2023
12 tasks
@HaleySchuhl HaleySchuhl added enhancement Enhancements to existing features update Updates an existing feature/method labels Jul 27, 2023
@HaleySchuhl HaleySchuhl added this to the PlantCV v4.0 milestone Jul 27, 2023
@HaleySchuhl HaleySchuhl self-requested a review July 27, 2023 17:45
@HaleySchuhl
Copy link
Contributor

Can you ping me when this is ready to review? 💡

@HaleySchuhl HaleySchuhl removed their request for review July 27, 2023 17:46
@JTvD
Copy link
Collaborator Author

JTvD commented Jul 27, 2023

Big thanks for all the help. A short status update from my side. We collected the new testdata. This looks a lot better, next week I have a meeting planned to ask your questions and go over the equations one more time. I hope to know more then, fingers crossed that everything adds up.

@JTvD
Copy link
Collaborator Author

JTvD commented Aug 1, 2023

Hello @nfahlgren,
It has been an interesting few days here, first we collected new test data which looked good on first sight.
However, in the pcv.visualize.chlorophyll_fluorescence plot of the light adapted plant, added below, my colleague saw that the Fmp has a lower value than the last frame. This means the lighting used to take the Fmp was not saturating.
image
We will be scanning to many plants to analyze all these plots, but it could be used to get a fast overview.

Not sure why, but phenovation called the PAM variant of NPQ is called the PAM time protocol.
image
Both the OJIP & PAM variant generate special filetypes on our cropreporter.
We have not tried these modes yet, I will ask if it's useful to add them already.

Regarding the extra plots, we rarely look at individual plants. Usually we track dozens of plants over time, after computing the PSII or NPQ some statistical correction is applied. All these measurements are than plotted in a single graph to check for differences between treatments.

There are a few more parameters which are often computed:

  • Linear electron flow
  • Energy-dependent quenching (Qe)
  • Photo inhibitory quenching (Qi)
    These are a bit debated so I am sure if it's wise to add them to the repository?
    Its also very simple to compute these parameters and many more ourselves when we have the frames in the PSII_data class.

@nfahlgren
Copy link
Member

Hi @wurDevTim, we could definitely circle back around to the PAM time protocol data. Some folks here are interested in Qe, etc., so worth adding if you also think it's useful for what you're doing. Thanks!

@nfahlgren nfahlgren merged commit d0debfd into danforthcenter:release-4.0 Aug 10, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancements to existing features update Updates an existing feature/method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants