atmoschem / eixport

Export Emissions to Atmospheric Models
https://atmoschem.github.io/eixport/
Other
27 stars 10 forks source link

units problem #54

Closed mdsumner closed 3 years ago

mdsumner commented 4 years ago

This example in to_munich() fails because path_ is an integer in new silicate:

#' data(emisco)
#' emisco <- emisco[1:100, "V8"]
#' dfco <- sfx_explode(emisco)
#' etm <- to_munich(sdf = dfco)

It's causing an error on CRAN submission for silicate:

https://win-builder.r-project.org/incoming_pretest/silicate_0.3.0_20200321_221157/reverseDependencies/summary.txt

I don't really understand how you were able to update the package in the last few days given the error with crs in silicate, but here breaks the assumption in to_munich() that everything has "units".

## this code now has `path_` to identify which part each segment came from (a problem in silicate before)
  segs <- tidyr::unnest(sc$object, cols = c("topology_"))

## ...
  ## if any object attributes remaining stick them onto the sf
  if (dim(segs)[2] > 0) {
    ## add columns back in
    sflines[names(segs)] <- segs
  }

I suggest either , don't error on everything having "units" - or edit the above line to be

  segs <- tidyr::unnest(sc$object, cols = c("topology_"))[c(".vx0", ".vx1")]

submitted as a PR #55

mdsumner commented 4 years ago

Maybe ignore the non-units column in to_munich()? Perhaps more flexibility is ok when a user can pass in any sf data frame?

(edited for tone)

mdsumner commented 4 years ago

Sorry for breaking eixport on CRAN, they asked me for justification and it was accepted. What I don't understand is how you've been able to submit updates in recent weeks, and what happened to the silicate/sf breakage. But, all good from my perspective - happy to help if problems arise.

ibarraespinosa commented 4 years ago

Hello @mdsumner Somehow I did not see your messages. I believe around March 22 I was moving from Sao Paulo to south Brazil with limited internet connection. Thank you very much with your PR. I remember that I had some problems with this function and CRAN. Around that time I didi not have time to dive deeper into the problem, but I remember that I waited your new silicate release and magically solve to_munich problems :D

I`m not using to_munich too often, hence I havent see this. I will check this in the following days. Thank you so much

ibarraespinosa commented 4 years ago

Actually, the last version was submitted on April 16, and the requirements are: silicate (≥ 0.3), sfheaders (≥ 0.2.1)

https://cran.r-project.org/web/packages/eixport/index.html