pecan icon indicating copy to clipboard operation
pecan copied to clipboard

Fixed Global function and variables from PEcAn.emulator

Open nanu1605 opened this issue 3 years ago • 5 comments

nanu1605 avatar Jul 24 '22 16:07 nanu1605

@mdietze In case when no arguments passed for eg in line 105 of modules/emulator/R/predict.GP.R should we use utils::setTxtProgressBar() directly? as we can not determine the min and max value

nanu1605 avatar Jul 29 '22 19:07 nanu1605

@nanu1605 1) if we switch to a new function we need to switch to that new functions args/usage, we can't just keep using it like the old function, nor can we use the function for updating the progress bar without defining it 2) in this case we most definitely do know the min, max, and state -- if you look on line 156 you see that the counter goes up to max = length(samp) and the counter variable is i instead of g

mdietze avatar Jul 29 '22 20:07 mdietze

@infotroph I think changing pda.calc.llik.par to PEcAn.assim.batch::pda.calc.llik.par is creating a circular dependency between emulator and assim.batch. Should we move the function to some other package?

nanu1605 avatar Aug 14 '22 08:08 nanu1605

Poking around a bit I think the best option to remove the circular dependency would be to move the functions in minimize.GP.R that use pda.calc.llik* into assim.batch, but I haven't fully checked yet whether that would just create a different circular dependency in the other direction if those functions are used elsewhere in emulator.

mdietze avatar Aug 14 '22 13:08 mdietze

@mdietze I think we can move the whole file (minimize.GP.R) to assim.batch as all the functions in the minimize.GP.R are only used in assim.batch.

nanu1605 avatar Aug 14 '22 21:08 nanu1605

@nanu1605 I resolved the conflicts from merging #3027 and added a few more cleanup items to get checks passing -- can you please review my commits from today?

I also left some suggestions above on the txtProgressBar calls -- I think they should be initialized once before the loop and then set inside it, not reinitialized.

infotroph avatar Oct 13 '22 05:10 infotroph

Sure @infotroph, I will do it ASAP

nanu1605 avatar Oct 13 '22 13:10 nanu1605

@mdietze I think we've addressed your change requests and this is ready for re-review

infotroph avatar Oct 20 '22 03:10 infotroph