danforthcenter / plantcv

Plant phenotyping with image analysis
Mozilla Public License 2.0
637 stars 263 forks source link

Typo in read_cropreporter example #978

Open wurDevTim opened 1 year ago

wurDevTim commented 1 year ago

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

HaleySchuhl commented 1 year ago

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!

nfahlgren commented 1 year ago

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).

wurDevTim commented 1 year ago

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:

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?

nfahlgren commented 1 year ago

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.

wurDevTim commented 1 year ago

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:

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
wurDevTim commented 1 year ago

@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.