plantcv icon indicating copy to clipboard operation
plantcv copied to clipboard

Typo in read_cropreporter example

Open JTvD opened this issue 2 years ago • 6 comments

Additional context Using plantcv 13.4.1 on a windows 10 machine.

Describe the bug In the documentation: https://plantcv.readthedocs.io/en/4.x/photosynthesis_read_cropreporter/ The function is called as follows:
ps = pcv.photosynthesis.read_cropreporter(filename="PSII_HDR_020321_WT_TOP_1.INF")

If we than look in the function we see it expects to be called with the '.DAT' file, line 28,29 of https://github.com/danforthcenter/plantcv/blob/main/plantcv/plantcv/photosynthesis/read_cropreporter.py

    inf_filename = filename.replace("PSD", "HDR")
    inf_filename = inf_filename.replace(".DAT", ".INF")

Expected behavior

  • The example is updated to call the function with a '.DAT' file to avoid confusion: ps = pcv.photosynthesis.read_cropreporter(filename="PSII_PSD_020321_WT_TOP_1.DAT")
  • Our cropreporter also outputs 'CHL', which contain a similar content. It would be nice if you either add support for this (new feature) or add a note that only 'PSD' files are supported.

JTvD avatar Nov 08 '22 08:11 JTvD

Thanks for bringing this to our attention @wurDevTim !

I was curious if this typo was showed up anywhere else, and it looks like the 4.x branch has this typo two places (also here) ./workflowname.py -i PSII_HDR_020321_WT_TOP_1.INF -o /home/user/output-images -D 'print'. The rest of the tutorial looks fine, but I think it's about time we do an annual comb through for any updates that didn't get propagated down into tutorials. Please let us know if you come across anything else like this!

HaleySchuhl avatar Nov 08 '22 14:11 HaleySchuhl

I don't think there is a typo. In PlantCV v3 the input file is the DAT file, but in v4 it is the INF file. In the links above you are comparing the documentation from 4.x to the code in the main branch, which is v3. We had to make this change because in v3 we only supported reading the PSD measurement file but in v4 we can read PSD, PSL, CLR, CHL, etc.

In the 4.x branch we have been developing a v4 release, and in v4 the photosynthesis submodule was completely redeveloped and supports more of the measurement routines from CropReporter systems. Versus in v3 we only supported Fv/Fm.

The 4.x branch is relatively stable and since you need the fuller supported set of measurements I would recommend using it. You will need to follow the installation from source instructions (https://plantcv.readthedocs.io/en/stable/installation/#installation-from-the-source-code) and checkout the branch 4.x (instead of the default main).

nfahlgren avatar Nov 08 '22 16:11 nfahlgren

Thank you for the explanation @nfahlgren. I have setup the 4.x branch as you recommended. Unfortunately I ran into exactly the same issue with the data from our phenovation cropreporter (recently installed on the campus of the Wageningen University). Over the past days I have been discussing with colleagues and it looks like our systems uses a different naming convention:

  • CHL_68_NPEC52.20220916AP.BM3.LK061.Control.3_1124.DAT
  • HDR_68_NPEC52.20220916AP.BM3.LK061.Control.3_1124.INF
  • CLR_68_NPEC52.20220916AP.BM3.LK061.Control.3_1124.DAT
  • PMD_68_NPEC52.20220916AP.BM3.LK061.Control.3_1124.DAT

We suspect there are more small changes between how the systems work. Instead of building our own tools, we would like to use plantcv to process our data. Would you be open to codeveloping the code to make it work for both our systems?

JTvD avatar Nov 16 '22 15:11 JTvD

Hi @wurDevTim, absolutely! We definitely want these modules to work across systems.

From the looks of it I think the only issue is that our system calls the dark-adapted measurements PSD (photosysnthesis dark?) and on yours PMD (pulse-modulation dark?), but my assumption is that these are the same protocol (could be wrong though).

In one sense it is sort of easy to fix because we recreate the DAT filepaths from the INF filepath and the particular line that is causing the issue is read_cropreporter.py L69. On L70 we check if the constructed path exists, maybe the easy solution is to try the same process with dataset="PMD" if the PSD file does not exist.

nfahlgren avatar Nov 16 '22 18:11 nfahlgren

Heey @nfahlgren, It's great to hear you want to cooperate. The cropreporter in our facility supports both PAM (Pulse-Amplitude-Modulation) and OJIP protocols:

  • The OJIP protocol creates a PSD or PSL file.
  • The PAM protocol creates a PMD or PML file.
    We are currently measuring with light/dark adapted PAM, but can also switch to OJIP and use NPQ.

While I think support for these protocols would be a nice addition to plantcv. It's also a bit more work to implement as some processing steps are different. If you want we can exchange contact details, data files, code or setup a meeting to discuss how to do this?

A smaller issue which blocks us from using it with out OJIP output is that platcv uses filename_components[1] = dataset # replace header with bin img type at line 257 of read_cropreporter

We don't know why but our systems has the 'dataset' as first part: filename_components[0]. Maybe it's possible to universalize the code to something like:

for i in range(0,len(filename_components)):
        if filename_components[i] == 'HDR':
            filename_components[i] = dataset  # replace header with bin img type

JTvD avatar Nov 17 '22 07:11 JTvD

@nfahlgren, @HaleySchuhl please take a look at my pull request: https://github.com/danforthcenter/plantcv/pull/984

After we have tested and merged this request I would also like to add support for the new NPQ, PAM and PAMtime protocols.

Hope to hear soon from you.

JTvD avatar Nov 29 '22 10:11 JTvD