Make `BADM_IC_process()` use PEcAn Settings / MultiSettings consistently
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 handlesis.Settings()/is.MultiSettings()type checking- Update tests to ensure this consistent behavior across single-site and multi-site configurations.
Hi, I went through the current implementation and agree that soilgrids_ic_process() uses ad-hoc logic to distinguish single vs multi-site runs.
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$sizefrom 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?
This looks to me like a good place for PEcAn.settings::papply()
@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.
hi @divine7022 i would like to work on this issue please assign it to me thanks !!
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...
apologize i missed track on this; does still anyone wanna work on this ?
Hi @divine7022 i would like to work please assign it thanks !!