pecan icon indicating copy to clipboard operation
pecan copied to clipboard

Make `BADM_IC_process()` use PEcAn Settings / MultiSettings consistently

Open divine7022 opened this issue 3 weeks ago • 3 comments

Description

BADM_IC_process() in modules/data.land/R/IC_BADM_Utilities.R currently implements its own ad‑hoc logic to distinguish single‑site vs multi‑site runs by checking for a top‑level "run" element and then wrapping settings in list(settings)

Proposed Refactor

  • Use PEcAn.settings::papply() instead of manual normalization
  • papply() internally handles is.Settings() / is.MultiSettings() type checking
  • Update tests to ensure this consistent behavior across single-site and multi-site configurations.

divine7022 avatar Dec 05 '25 00:12 divine7022

Hi, I went through the current implementation and agree that soilgrids_ic_process() uses ad-hoc logic to distinguish single vs multi-site runs.

Image

I can refactor it to use PEcAn.settings::is.Settings() / is.MultiSettings() for proper type handling by:

  • Normalizing input at the start
  • Processing settings consistently as a list
  • Reading ensemble$size from the normalized settings

This should simplify the code and align it with other PEcAn modules. I’ll also update tests for both single and multi site runs.

Should I open a PR with this refactor? Could you please assign this issue to me?

shivpratikhande avatar Dec 06 '25 08:12 shivpratikhande

This looks to me like a good place for PEcAn.settings::papply()

infotroph avatar Dec 06 '25 11:12 infotroph

@shivpratikhande that could be an another refectory of soilgrids_ic_process() i need to take a look. but the issue says a different function; I apologize to pointing to wrong file modules/data.land/R/IC_SOILGRID_Utilities.R it's modules/data.land/R/IC_BADM_Utilities.R.

thanks @infotroph for heads up -- PEcAn.settings::papply() handly this internally

This looks to me like a good place for PEcAn.settings::papply()

updated the description.

divine7022 avatar Dec 06 '25 12:12 divine7022

hi @divine7022 i would like to work on this issue please assign it to me thanks !!

ayushman1210 avatar Dec 14 '25 01:12 ayushman1210

Hi @divine7022 I’d love to work on this issue if it’s still open..! From what I understand, BADM_IC_process() currently has its own way of handling single-site vs multi-site runs (like checking settings$run and wrapping things in a list), even though PEcAn already has utilities for this. Using PEcAn.settings::papply() should make the code cleaner and keep the behavior consistent with other modules.

My plan would be to switch over to papply(), remove the manual handling, and make sure the existing IC logic runs cleanly for both single-site and multi-site cases. I’ll also update or add tests to cover both setups.

I’m happy to move forward with the fix if that sounds good...

Akash-paluvai avatar Dec 17 '25 16:12 Akash-paluvai

apologize i missed track on this; does still anyone wanna work on this ?

divine7022 avatar Dec 24 '25 18:12 divine7022

Hi @divine7022 i would like to work please assign it thanks !!

ayushman1210 avatar Dec 24 '25 21:12 ayushman1210