Review plant_height script output

craig-willis commented 6 years ago

Review task for plant_height script and output: https://github.com/terraref/extractors-3dscanner/blob/master/plant_height/laser_to_height.sh

We seem to have lost track of this review task in the extractor repo: https://github.com/terraref/extractors-3dscanner/issues/9#issuecomment-345720796

Note:

This script is not an extractor
The script operates on the raw PLY data (in Level 1), not the merged LAS data

Completion criteria:

[x] Code review complete (no extractor, so just script)
[x] Sample output data generated
[x] Sample output data verified
[x] Changes for 2017 release implemented
[ ] Issues for 2018 created
[ ] Code tagged/versioned
[ ] Script executed
[ ] Final output verified/reviewed

craig-willis commented 6 years ago

Per https://github.com/terraref/extractors-3dscanner/issues/9#issuecomment-345720796:

The first end-to-end test ran over the weekend on ROGER for 2017-06-02. Please check that the Level_1 and trait data are as expected.

Data is on ROGER: /projects/arpae/terraref/sites/ua-mac/Level_1/canopy_height/

dlebauer commented 6 years ago

(from https://github.com/terraref/extractors-3dscanner/issues/9#issuecomment-348633347)

in the Level 1 data, I see 359 unique dates under scanner3DTop and 63 unique dates under scanner3DTop_mergedlas
in BETYdb, there appear to be some very small values from May 30 through June 2 2017

ZongyangLi commented 6 years ago

@dlebauer Is that means only some of data points have such a very small and negative values? PI643008 is a gene id?

craig-willis commented 6 years ago

@dlebauer What is the relation to mergedlas? I think this script operates only on the PLY files

dlebauer commented 6 years ago

@ZongyangLi PI643008 is a cultivar (== genotype) id.

@craig-willis sorry I missed that.

dlebauer commented 6 years ago

@ZongyangLi It looks like ther are no longer any records < 0 but all of the records with the 'Scanner 3d ply data to height' method are <1.1 cm

ZongyangLi commented 6 years ago

@dlebauer Last time when we discuss about unit for canopy height: https://github.com/terraref/computing-pipeline/issues/210#issuecomment-300318882, I was thinking I should update the output unit to meter instead of cm.

So it's in meter now. I am sorry if I make another misunderstanding, should I change it for cm?

dlebauer commented 6 years ago

@ZongyangLi yes please change to cm. It is an option to change all of our height data to m but at this point we have been using cm, so the easiest at this point is to use cm, and if it would be useful to change to m, we can decide with inputs from other groups and convert all of the code and data at once.

dlebauer commented 6 years ago

I purged the old data

delete from traits 
   where method_id = (select id from methods where name = 'Scanner 3d ply data to height')

craig-willis commented 6 years ago

@dlebauer Should we run another sample data run, or start full processing? (i.e., can we check more boxes in the completion criteria)

dlebauer commented 6 years ago

@craig-willis yes, everything else looked good; should be ready to go

craig-willis commented 6 years ago

@ZongyangLi There seems to be a problem handling season 1 data

        # MAC Field Scanner Season 4 Range 5 Column 6 W
        plot_record = [int(s) for s in site.split() if s.isdigit()]
        if len(plot_record) != 3:
            return 0, 0, 0, 0, 0, 0

This won't work for the season 1 names, for example: "MAC Field Scanner Season 1 Field Plot 542 E".

Is this expected?

ZongyangLi commented 6 years ago

@craig-willis That's right, it can not handle that. I didn't imagine that plot records have different style, shouldn't it be constant?

Besides, I don't think season 1 or season 2 should be involve, since the laser scanner's offset(https://github.com/terraref/reference-data/issues/44) is not constant during the first 2 season.

craig-willis commented 6 years ago

@dlebauer Is there any additional action to be taken here? I assume you want me to move forward with running on the later seasons.

dlebauer commented 6 years ago

Yes please run the later seasons. Ignore the season 1 names that do not contain range and column. I was not aware that the laser offset changed during the first two seasons. Is there an open issue that addresses this?

On Wed, Dec 6, 2017 at 11:39 AM Craig Willis notifications@github.com wrote:

@dlebauer https://github.com/dlebauer Is there any additional action to be taken here? I assume you want me to move forward with running on the later seasons.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/terraref/reference-data/issues/210#issuecomment-349716944, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcX55Il8Cqz9mmv-KJmo61qsZnLE3uYks5s9tDTgaJpZM4Q03-l .

ZongyangLi commented 6 years ago

I could not find such an issue now, maybe I am wrong. The offset is constant in all the field seasons. Then we don't need to worry about this.

terraref / reference-data

Review plant_height script output #210