Remove dependency on `data.atmosphere` from `data.land`

Sweetdevil144 commented 1 month ago

Updated corresponding changes to CHANGELOG.md as per suggestions

Description

Replaced the PEcAn.data.atmosphere::db.site.lat.lon function with PEcAn.DB::query.site for retrieving site latitude and longitude in the soil_process function.

Motivation and Context

Review Time Estimate

[ ] Immediately
[x] Within one week
[ ] When possible

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My change requires a change to the documentation.
[ ] My name is in the list of CITATION.cff
[x] I have updated the CHANGELOG.md.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

Sweetdevil144 commented 1 month ago

@infotroph was correct. Seems like I failed to push my changes after I only committed them. My fault :) . More clarity into the above changes @mdietze . To check whether The above changes were valid or not, I created my custom test case as below, other than make test to be ran :

test_that("query.site and db.site.lat.lon return the same results", {
  db_params <- PEcAn.DB::get_postgres_envvars(
    dbname = "bety",
    user = "bety",
    password = "bety",
    host = "localhost",
    driver = "Postgres"
  )
  con <- PEcAn.DB::db.open(db_params)
  site <- list(id = 1, sitename = "Aliartos")

  latlon1 <- PEcAn.DB::query.site(site$id, con = con)[c("lat", "lon")]
  latlon2 <- PEcAn.data.atmosphere::db.site.lat.lon(site$id, con = con)
  expect_equal(latlon1$lat, latlon2$lat)
  expect_equal(latlon1$lon, latlon2$lon)

  PEcAn.DB::db.close(con)
})

Results were as follow :

root@3c52500f363a:/pecan# Rscript -e "testthat::test_file('modules/data.atmosphere/tests/testthat/test-site-latlon.R')"

══ Testing test-site-latlon.R ════════════════════════════════════════════════════════════════
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 2 ] Done!

Sweetdevil144 commented 1 month ago

By @infotroph in Slack,

I disagree — seems safe to deleting it after replacing all the uses of db.site.lat.lon inside met.process with query.site same as you’re doing in data.land.

If we're doing this Fix, then shouldn't we add the above recommended changes too in this PR?

infotroph commented 3 weeks ago

@Sweetdevil144 Last step is to remove PEcAn.data.atmosphere from the DESCRIPTION file of PEcAn.data.land, then run ./scripts/generate_dependencies.R to propagate that change through the dependency lists.

Sweetdevil144 commented 3 weeks ago

Not sure why the tests fail even now?

infotroph commented 3 weeks ago

@Sweetdevil144 The tests are failing because we use exact string matches to ignore an existing warning that "Imports includes 35 non-default packages.", and you just improved it to "Imports includes 34 non-default packages." 🤣

This is one of the rare-ish cases where the right fix is to manually edit the saved check outputs. Will you please add a commit that changes 35 to 34 on line 16 of modules/data.land/tests/Rcheck_reference.log (and does nothing else)?

Sweetdevil144 commented 3 weeks ago

we use exact string matches to ignore an existing warning

That's my first time seeing this in PEcAn atmosphere 🥲. I've been debugging what's wrong since morning. Thanks a lot for the aid !!

infotroph commented 3 weeks ago

I've been debugging what's wrong since morning

I'm sorry for the frustration! You're not alone: This cached-result-comparison step is way too complicated and causes a lot of grief for the whole team. Has it produced enough of an increase in code quality to be worth it? Weeeeell... we hope so but can't prove it.

PecanProject / pecan