Repository navigation
sipnet: merged segments get the literal string "NA" in crop_code (segments.csv) #4183
Description
Summary
Follow-up to #4122 / #4129. drop_same_pft_segments() in models/sipnet/R/write_segmented_configs.R builds the merged crop_code with paste(.data$crop_code, collapse = " "). A missing code is pasted as the text "NA".
The first segment (the period before the first planting) always has crop_code = NA (segment_dataframe()), so when it is merged with a following segment of the same PFT, segments.csv gets a value like "NA D12".
Reproduction (on develop)
seg <- tibble::tibble(
start_date = as.Date(c("2025-01-01", "2025-01-04", "2025-01-08")),
end_date = as.Date(c("2025-01-03", "2025-01-07", "2025-01-10")),
crop_code = c(NA, "D12", "D13"),
pft = c("soil", "soil", "crop"),
segment_id = c("001", "002", "003")
)
PEcAn.SIPNET:::drop_same_pft_segments(seg)
#> first row: crop_code = "NA D12", pft = soil, 2025-01-01 to 2025-01-07Impact
Cosmetic: crop_code is only written to segments.csv and is not used elsewhere in the package, so run results are not affected. But the file then contains a code, "NA", that is not a real crop.
Possible fix
paste(stats::na.omit(.data$crop_code), collapse = " ") (an all-missing group would give "", which could be turned back into NA_character_). I'm happy to send a small PR with a test if this is wanted.
Also
It might be worth a test through write_segment_configs() showing that a merged segment's inputs still cover its full date range.
Activity
madhavzanwar commentedon Oct 23, 2026
@infotroph filed as requested. Happy to send a small PR if you’d like.
infotroph commentedon Oct 23, 2026
Thanks for noticing this and for the compact reprex!
I think keeping "NA" in the crop_code string is basically correct for the use case I expect, which is basically just human readers who want a quick check what crop history is represented in the simulation. "This segment arrived with a PFT label but not a crop code" seems informative enough to be worth keeping, especially in a case like this where keeping it is less work than removing it 😃
Yes, it would. The existing test of drop_same_pft_segments checks this implicitly for one input by matching exact dates ing every row, but I'd be glad to review a PR that adds a separate check making it explicit.
madhavzanwar commentedon Oct 24, 2026
Understood, thanks, I’ll leave the "NA" as is. I’ll send a PR that only adds a test through write_segment_configs() checking that merged segments still cover their full date range.
madhavzanwar commentedon Oct 25, 2026
@infotroph the test you offered to review is up: #4190
Simplify test: add expectations to the drop_same_pft_segments test
Merge branch 'develop' into PecanProjectGH-4183-segment-merge-test
Test that all-same-PFT segments collapse to one row
Test that write_segmented_configs merges same-PFT crops end to end
Merge pull request PecanProject#4190 from madhavzanwar/PecanProjectGH…