NEON tools not handling crop weights correctly

[wwieder]

Brief summary of bug

@negin513 and I noticed that the current subset_data and modify_singlept_site_neon scripts don't set 'PCT_CFT' correctly.

General bug fix

#1461 should be modified with the simple example below, where a use defines a single dominant PFT, which gets 100% weight on respective land units (@ekluzek edited this based on our discussion).

if dompf < 15:
PCT_NAT_PFT(dompft) = 100
else:
PCT_CFT(dompft-15) = 100

Land unit weights should also be adjusted defined by PCT_NATPFT &PCT_CROP
if dompf < 15:
PCT_NATVEG = 100
PCT_CROP = 0
else:
PCT_NATVEG = 0
PCT_CROP = 100

[wwieder]

Main questions:

1. Are we OK with only allowing one land unit (pct_natpft or pct_crop) if users are defining dompft in the subset_data. (Note, this makes 2 simpler). (yes we like @negin513 interface for this)
2. Does our proposed fix make sense? (Code currently can handle different weights of pct_nat_pft or pct_cft on respective land units) (yes, I corrected above, based on our discussion)
3. Is indexing of cfts correct based on this plan? (yes)

[ekluzek]

@wwieder and @negin513 thanks for digging into this. (I edited my answer to correct my initial take which was wrong).

Iit's PCT_NATVEG rather than PCT_NATPFT, and you want the equality to be

if dompft <= 14:

(or if dompft < 15:)

You want all PFTS from 0 to 14 to be expressed as natural PFT's. The crops start at "PFT" 15 and up. With index 15 and 16 being generic C3 crop (rainfed and then irrigated). The PCT_NATPFT array is zero based (with 0 as bare soil), but the PFT_CFT array has temperate_rainfed_corn as index 0 (in python). It's easy to get this off by 1.

[ekluzek]

Several of us met last Friday to talk about this and go over the details (@negin513 @wwieder @danicalombardozzi and I). I had thought that my answer above was correct, but in our discussion I found I was off. The two generic crops are still considered CFT's (since they will be on their own crop landunit). So the natural veg PFT's are 0 to 14 (with 15 PFT's). So the inequality should be <= 14 or < 15. We should also make sure that the magic number 15 is encapsulated as a parameter in the code. I'll edit my answer above to give the correct one.

We also found that the details of this are tricky to get right. There are issues like python does 0 based array subscripts, while FORTRAN does 1-based, but even in the FORTRAN code we start PFT's at 0 to signal bare-soil. But, CFT's start at index 1 for rainfed generic C3 crop. @danicalombardozzi found it tricky to get this all right in her analysis code.

That made me realize that we really need to have some unit-testing for this in place to make sure this tricky logic is handled correctly, and stays correct as the code gets changed. @negin513 and I talked about that and will be working on that in #1461. Anytime there is sensitive tricky logic we really need to have unit-testing around it to make sure it's correct and stays correct. As the adage goes: If you don't test it -- it's broken. And if you don't continue to test it -- it'll get broken.

[wwieder]

sounds good. thanks for looking into this!