search

pik-piam / magclass

R package | Data Class and Tools for Handling Spatial-Temporal Data
GNU Lesser General Public License v3.0
4 stars 24 forks source link

conversion magclass <-> RasterBrick #78

Closed tscheypidi closed 3 years ago

tscheypidi commented 3 years ago

added conversion functions for magclass <-> RasterBrick (method for as.magpie as well as as.RasterBrick)

In addition: added new, list-based subsetting method for magclass objects of the form x[,,list("bla","blub","ble")] which can replace stacked subsetting constructs of the form x[,,"bla"][,,"blub"][,,"ble"]

codecov[bot] commented 3 years ago

Codecov Report

Merging #78 (f54b897) into master (d11fe7b) will increase coverage by 1.30%. The diff coverage is 80.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   26.54%   27.85%   +1.30%     
==========================================
  Files          96       99       +3     
  Lines        4083     4147      +64     
==========================================
+ Hits         1084     1155      +71     
+ Misses       2999     2992       -7
Impacted Files Coverage Δ
R/collapseNames.R 66.66% <ø> (ø)
R/setItems.R 0.00% <0.00%> (ø)
R/collapseDim.R 71.42% <71.42%> (ø)
R/magpie-class.R 45.23% <75.00%> (+5.23%) :arrow_up:
R/getItems.R 90.47% <85.36%> (+0.62%) :arrow_up:
R/as.RasterBrick.R 86.66% <86.66%> (ø)
R/as.magpie.R 50.58% <91.66%> (+7.73%) :arrow_up:
R/getCoords.R 70.00% <100.00%> (ø)
R/hasCoords.R 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d11fe7b...f54b897. Read the comment docs.

tscheypidi commented 3 years ago

Thanks for that detailed response!

I started to review. Everything seems working for the test cases specified in the help part of the function. I would like to test it for a raster I have used in another context for a final approval.

  • getItems is now quiet complex I didn't overlook all changes I have to admit. But I like that it is possible to rename stuff now. the following things I noticed:.

    • You cannot add dims anymore via renaming. At least before this was possible. Maybe it is good to forbid it, but probably that reduced the willingnes to adopt it.

You can now add new dimension via calling a subdimension which does not exist yet. Doesn't that cover the old use case?

  • What is still missing is an as.integer flag for the years (which is a thing that at least I use frequently).

This is very specific for the time dimension. I do not plan to delete the other functions in the next time, so I think for these cases a direct call of getYears would be my suggestion.

  • also a setItems would be good. added it.

  • the maindim argument I do not fully understand (despite the long explanation - sorry -.-) not sure how to explain it better. Its purpose is basically to define in which main dimension the changes should be applied. Usually the dimcode tells the maindimension (3.1 is in main dimension 3), but sometimes this information is missing (e.g. if you ask getItems to add dimension "bla". In that case it is not clear, whether "bla" should be spatial, temporal, or data.... Any suggestion how to explain it better in the documentation?

  • as.RasterBrick seems fine. Only: I wouldn't use colnames in line 40, but rather magclass internal functions like getItems. If I am not mistaken the return value of the wrap function is not necessarily a MAgPIE object anymore, which is why I am using colnames here.

  • as.magpie for RasterBricks need documentation (or maybe I overlooked it). At the moment the naming convention of the RasterBrick has to be quite strict to get as.magpie working on it. I am also not 100% sure about the conditional expression from line 327-334. Why shouldn't there be a pure temporal raster brick? If this would be the case this would be transformed to the third dim in a mag object or you would have to change the name artificially before hand. I would additionally test for 4 digits, if ncol()==1, to specify temporal.

I am unsure where and how I can document that, but the conversion should work for most raster object in the first place. Only if you want to work with subdimensions the syntax becomes relevant, but this you can see if you convert a magclass object to raster first. The special case in line 327-334 does not mean that everything automatically goes to the data dimension. I call it data, because I have to give the dimension a name but do not have any details about it. Hence, in most cases the "data" guess might not be that bad. setting temporal to NULL means that tidy2magpie will analyze on its own what kind of dimension it is....Hence, if you have specified the entries year-like it should automatically detect it as temporal dimension.

  • I like collapseDim - however I guess it will take ages to get collapseNames removed. I don't plan to remove it in the near future, but I want to encourage everyone to use collapseDim for new developements as it is just more versatile....at some point when collapseDim has prove to work properly I might internally replace the sourcecode of collapseNames with a simple call to collapseDim (with the appropriate settings to resemble the behavior of collapseNames).
  • hasCoords is OK. At the same time I am not sure if testing the naming convention is more robust to missuse than testing the structure of the items with a regex (all Items should have a structure ike d,d.d,d or so). I mean everybody can say that his or her GLO.1 has the names x.y ^^'

Agreed. However, the current implentation has the advantage to being rather ressource efficient. If we run into cases where this kind of detection causes problems it should be easy to improve it/make it more strict.