femiguez / apsimx

R package for APSIM-X
https://femiguez.github.io/apsimx-docs/
49 stars 20 forks source link

Adding retrieving soil data through slga R package and also weather databases for Aus and NZ #53

Open femiguez opened 2 years ago

femiguez commented 2 years ago

This will take some time. The slga option seems straight forward

adamhsparks commented 3 months ago

Hey Fernando, I might be in a position to help with this issue now!

We've put together {weatherOz} and included functionality to fetch APSIM .met formatted data for Australian data sources, sorry NZ.

DPIRD weather stations in WA only - https://docs.ropensci.org/weatherOz/reference/get_dpird_apsim.html

SILO/BOM weather stations (national) - https://docs.ropensci.org/weatherOz/reference/get_patched_point_apsim.html

SILO/BOM interpolated weather (national) - https://docs.ropensci.org/weatherOz/reference/get_data_drill_apsim.html

We use your package to create and S3 object in the R session but don't do anything else with it, i.e., no writing facilities are provided, just fetching the data and creating one of your {apsimx} .met files with the data.

Would you like to include the functionality from {weatherOz} in {apsimx}? We could consider migrating it like what we did with {nasapower} where I deprecated it after you integrated it, but this is a bit more tricky since it's more baked in to {weatherOz} with the way that the SILO API works where you can just request an APSIM .met file as the format that you want returned.

For the DPIRD API it's more straightforward to integrate into {apsimx}, I borrowed one of your functions, amp_apsim_met(), to create the .met object and return it from the values that were fetched from the API. https://github.com/ropensci/weatherOz/blob/main/R/get_dpird_apsim.R.

Maybe it would work to re-export our fns in {apsimx}?

This isn't on CRAN yet, but it's headed there soon. We've put it through rOpenSci code review and is now waiting publication in JOSS.

femiguez commented 3 months ago

@adamhsparks This is great! I'll test it and review it and provide feedback when I can.

I just sent to CRAN a new version of the apsimx package (2.7.7). One of the changes are related to plots for 'met' objects. This is an example.

library(apsimx)
library(nasapower)
pwr <- get_power_apsim_met(lonlat = c(-93,42), dates = c("1990-01-01","2020-12-31"))
plot(pwr, plot.type = "anomaly", met.var = "rain")
plot(pwr, plot.type = "anomaly", met.var = c("rain", "avg_maxt"), summary = TRUE, years = c(1993, 2012))
adamhsparks commented 3 months ago

I just righted an embarrassing wrong. I'd used some of your code from {apsimx} and credited you in the function's documentation, but you weren't in the Author's list in the DESCRIPTION file. I've corrected that just moments ago. I am very sorry about that.

You appear in the authors' list now, https://docs.ropensci.org/weatherOz/authors.html!

adamhsparks commented 3 months ago
library(apsimx)
library(nasapower)
pwr <- get_power_apsim_met(lonlat = c(-93,42), dates = c("1990-01-01","2020-12-31"))
plot(pwr, plot.type = "anomaly", met.var = "rain")
plot(pwr, plot.type = "anomaly", met.var = c("rain", "avg_maxt"), summary = TRUE, years = c(1993, 2012))

Nice!

femiguez commented 3 months ago

Thanks for adding me to the list! As you say, one possibility is to move these functions to the apsimx package and then I would maintain them. I will like to test them and give more specific feedback (I don't have time right now), but it might be a good idea to use (import) functions such as 'as_apsim_met', 'tav_apsim_met' and 'amp_apsim_met'. Partly, because they might change. Only recently, I made changes and even APSIM made changes in the way they are calculated.

adamhsparks commented 3 months ago

There is no hurry at all. Thanks for the feedback.

adamhsparks commented 3 months ago

Looking at the codebase, I can't recall why I didn't just call amp_apsim_met(). I modified your amp_apsim_met() function, simplifying it down quite a bit and changing a few things, using sprintf() over paste() and the comment for "amp" being different.

I just import all other functionality. But I'll take another look at just importing amp_apsim_met() to see if I can't do that for the good reasons you've provided.

adamhsparks commented 3 months ago

OK, one function refactored!

I went back and re-evaluated get_dpird_apsim() and now use as_apsim_met() directly imported and have simplified the codebase of that function greatly! It improves it since it uses your calculations and checks built in to {apsimx}.

I think the SILO functions are less problematic since SILO returns a .met formatted character object that I just read from tempdir() using read_apsim_met(). Some users have asked for the comments from SILO to be maintained, which I'll handle in the API client to ensure that they comments are included when the file is written to disk by manually reinserting the comments attribute that is stripped out on import.

adamhsparks commented 3 months ago

I've refactored the second set of functions.

All functions now take advantage of your diagnostics with as_apsim_met() to convert a data.frame and I don't duplicate any of your functionality (even slimmed down), it's all an import to ensure that we get your latest versions.