Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move mr packages to Imports #578

Merged
merged 5 commits into from
Oct 24, 2024
Merged

move mr packages to Imports #578

merged 5 commits into from
Oct 24, 2024

Conversation

fbenke-pik
Copy link
Contributor

@fbenke-pik fbenke-pik commented Oct 16, 2024

This PR aims to reduce the number of Depends packages without losing madrat functionality.
For more info on motivation, see: pik-piam/madrat#219

@tscheypidi Let me know if you see any problems with that approach. Otherwise, I would opt for this style of madrat attaching for mrremind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why not move madrat and magclass, to? :p
  2. What bug did this fix exactly? Or is everybody just ignoring the version number scheme?
  3. Waiting for /p/tmp/pehl/pre-processing/log-2024-10-16-1182152.out.

@fbenke-pik
Copy link
Contributor Author

fbenke-pik commented Oct 16, 2024

Why not move madrat and magclass, to? :p

I just did locally, but now I get a troubling warning:

2: In getMadratGraph(...) :
  Following functions could not be found in the scope of packages to be checked.: 
   calcFAOLand->calcBiomassPrices, calcLanduseInitialisation->readREMIND_11Regi, calcFAOLand->readStrefler, calcFAOLand->toolBiomassSupplyAggregate, calcEdgeTransportSAinputs->toolLoadmrtransportData, readFAO_online->calcAgProductionValue, readFAO_online->calcAnimalStocks, calcLanduseInitialisation->calcAtmosphericDeposition, calcLanduseInitialisation->calcBiomeType, calcAttributes->calcBioplasticToBiomass, calcFAOmassbalance_pre->calcCentralFeedshares, calcLanduseInitialisation->calcEmisNitrogenPreagriculture, calcCroparea->calcFAOIntraYearProd, readFAO_online->calcFAOIntraYearProd, calcAttributes->calcFAOIntraYearProd, calcFAOmassbalance_pre->calcFAOmassbalance, calcAttributes->calcFAOmassbalance, calcCroparea->calcFAOYield, calcCropareaLandInG->calcFAOYield, calcAttributes->calcFeedBalanceflow, calcFAOmassbalance_pre->calcFeedBalanceflow, calcFAOmassbalance_pre->calcFeedBaskets, calcFAOmassbalance_pre-> [... truncated]

So maybe the onload approach does not work after all?

@fbenke-pik fbenke-pik marked this pull request as draft October 16, 2024 15:58
@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

I just did locally, but now I get a troubling warning:

2: In getMadratGraph(...) :
  Following functions could not be found in the scope of packages to be checked.: 
   calcFAOLand->calcBiomassPrices, calcLanduseInitialisation->readREMIND_11Regi, calcFAOLand->readStrefler, calcFAOLand->toolBiomassSupplyAggregate, calcEdgeTransportSAinputs->toolLoadmrtransportData, readFAO_online->calcAgProductionValue, readFAO_online->calcAnimalStocks, calcLanduseInitialisation->calcAtmosphericDeposition, calcLanduseInitialisation->calcBiomeType, calcAttributes->calcBioplasticToBiomass, calcFAOmassbalance_pre->calcCentralFeedshares, calcLanduseInitialisation->calcEmisNitrogenPreagriculture, calcCroparea->calcFAOIntraYearProd, readFAO_online->calcFAOIntraYearProd, calcAttributes->calcFAOIntraYearProd, calcFAOmassbalance_pre->calcFAOmassbalance, calcAttributes->calcFAOmassbalance, calcCroparea->calcFAOYield, calcCropareaLandInG->calcFAOYield, calcAttributes->calcFeedBalanceflow, calcFAOmassbalance_pre->calcFeedBalanceflow, calcFAOmassbalance_pre->calcFeedBaskets, calcFAOmassbalance_pre-> [... truncated]

So maybe the onload approach does not work after all?

Crap.
So, mrremind loads and registers mrcommons, which loads mrfaocore, but does not register it with madrat, because it does not get attached and does not trigger its madratAttach() call.

And /p/tmp/pehl/pre-processing/log-2024-10-16-1182152.out is silent on this issue, instead loading something from the cache:

Run calcOutput(type = "BiomassPrices", file = "f30_bioen_price.cs4r", round = 6)
…
~ Run calcOutput(type = "FAOLand", aggregate = FALSE)
~~  - forced cache does not match fingerprint ac62cfaa
~~  - loading cache calcFAOLand.rds
~ Exit calcOutput(type = "FAOLand", aggregate = FALSE) in 0.05 seconds
~  - writing cache calcBiomassPrices.rds
…
Exit calcOutput(type = "BiomassPrices", file = "f30_bioen_price.cs4r", round = 6) in 6.16 seconds

Leaves only option 1, which could be rolled out using lucode2::buildLibrary().

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

R CMD check is of the same opinion, by the way:

Depends: includes the non-default packages:
  'edgeTransport', 'GDPuc', 'madrat', 'magclass', 'mrcommons',
  'mrdrivers'
Adding so many packages to the search path is excessive and importing
selectively is preferable.

@fbenke-pik
Copy link
Contributor Author

fbenke-pik commented Oct 17, 2024

Crap. So, mrremind loads and registers mrcommons, which loads mrfaocore, but does not register it with madrat, because it does not get attached and does not trigger its madratAttach() call.

Are you sure? After my latest fixes on this branch, the warning is gone and the caching looks fine even for functions in mrlandcore, which is not directly imported. Also, we no longer use and Depends for any piam packages.

>madrat::getDependencies("calcBiomassPrices", direction = "din")
               func type   package                          call     hash
1       calcFAOLand calc mrfaocore       mrfaocore:::calcFAOLand 08f6c6e0
2        readMAgPIE read mrcommons        mrcommons:::readMAgPIE 9eac99bd
3    toolConvertGDP tool     GDPuc        GDPuc:::toolConvertGDP 84d0659f
4 toolGetUnitDollar tool mrdrivers mrdrivers:::toolGetUnitDollar 969314a3
Warning messages:
1: In .warnNotfound(getMappings) :
  Mapping in mrcommons:::readGTAPv8v9 not found!
2: In .checkBidirectional(out, details = FALSE) :
  Bidirectional package dependencies detected: mrremind->edgeTransport, edgeTransport->mrremind
  You might want to have a look at the following connections: readTransportSubsidies->toolLoadmrremindData
3: In .checkBidirectional(out, details = FALSE) :
  Bidirectional package dependencies detected: mrremind->edgeTransport, edgeTransport->mrremind
  You might want to have a look at the following connections: readTransportSubsidies->toolLoadmrremindDatacalcEdgeTransportSA->readEDGETransport
> madrat::getDependencies("calcBiomassPrices", direction = "in")
                                 func type    package                                           call     hash
1                  calcPopulationPast calc  mrdrivers                 mrdrivers:::calcPopulationPast b3880a03
2          calcInternalPopulationPast calc  mrdrivers         mrdrivers:::calcInternalPopulationPast 727352ff
3  calcInternalPopulationPastEurostat calc  mrdrivers mrdrivers:::calcInternalPopulationPastEurostat 52e5f620
4                         calcFAOLand calc  mrfaocore                        mrfaocore:::calcFAOLand 08f6c6e0
5           calcLanduseInitialisation calc mrlandcore         mrlandcore:::calcLanduseInitialisation d6eccf1d
6       calcLanduseInitialisationBase calc mrlandcore     mrlandcore:::calcLanduseInitialisationBase 0d638328
7                       calcLPJmL_new calc mrlandcore                     mrlandcore:::calcLPJmL_new 061e5785
8                          calcLUH2v2 calc mrlandcore                        mrlandcore:::calcLUH2v2 a367da9a
9                      calcForestArea calc mrlandcore                    mrlandcore:::calcForestArea 0f142a2c
10                         readMAgPIE read  mrcommons                         mrcommons:::readMAgPIE 9eac99bd
11                           readCEDS read  mrcommons                           mrcommons:::readCEDS c4c16311
12                     readPRIMAPhist read  mrcommons                     mrcommons:::readPRIMAPhist 21fbbd58
13                 readMissingIslands read  mrdrivers                 mrdrivers:::readMissingIslands e04c16ca
14                            readWDI read  mrdrivers                            mrdrivers:::readWDI dfb6a8ae
15                      readUN_PopDiv read  mrdrivers                      mrdrivers:::readUN_PopDiv 9db938da
16                 readEurostatPopGDP read  mrdrivers                 mrdrivers:::readEurostatPopGDP a28213c1
17                     readFAO_online read  mrfaocore                     mrfaocore:::readFAO_online c5372fc6
18                        readFRA2020 read  mrfaocore                        mrfaocore:::readFRA2020 21d51fbb
19                    readFAO_FRA2015 read  mrfaocore                    mrfaocore:::readFAO_FRA2015 b38c9389
20                         readLUH2v2 read mrlandcore                        mrlandcore:::readLUH2v2 4dbad153
21                      readLPJmL_new read mrlandcore                     mrlandcore:::readLPJmL_new 118102d8
22                    readLPJmLInputs read mrlandcore                   mrlandcore:::readLPJmLInputs 4dbcdcaf
23                     toolConvertGDP tool      GDPuc                         GDPuc:::toolConvertGDP 84d0659f
24                      toolAggregate tool     madrat                         madrat:::toolAggregate 7783f250
25                    toolCountryFill tool     madrat                       madrat:::toolCountryFill f02cc82a
26                     toolGetMapping tool     madrat                        madrat:::toolGetMapping b688b718
27                  toolSubtypeSelect tool     madrat                     madrat:::toolSubtypeSelect 86ae28b2
28                  toolISOhistorical tool     madrat                     madrat:::toolISOhistorical d32cab8f
29                toolCountry2isocode tool     madrat                   madrat:::toolCountry2isocode 22b0caa9
30             toolConditionalReplace tool     madrat                madrat:::toolConditionalReplace 7420eb71
31                      toolNAreplace tool     madrat                         madrat:::toolNAreplace b7eba472
32                     toolOrderCells tool     madrat                        madrat:::toolOrderCells 1f37ccd0
33                   toolSplitSubtype tool     madrat                      madrat:::toolSplitSubtype fe376d4d
34                    toolTimeAverage tool     madrat                       madrat:::toolTimeAverage 5e399f5d
35                     toolTimeSpline tool     madrat                        madrat:::toolTimeSpline bb79b84c
36                  toolGetUnitDollar tool  mrdrivers                  mrdrivers:::toolGetUnitDollar 969314a3
37                         toolReduce tool  mrdrivers                         mrdrivers:::toolReduce 881a2a95
38                 toolCheckUserInput tool  mrdrivers                 mrdrivers:::toolCheckUserInput 5d8931cf
39               toolFinishingTouches tool  mrdrivers               mrdrivers:::toolFinishingTouches 36c5d8fa
40                       toolFillWith tool  mrdrivers                       mrdrivers:::toolFillWith b751714d
41          toolGetScenarioDefinition tool  mrdrivers          mrdrivers:::toolGetScenarioDefinition 2bd10292
42                 toolGeneralConvert tool  mrdrivers                 mrdrivers:::toolGeneralConvert 5d6a17ee
43                 toolGetEUcountries tool  mrdrivers                 mrdrivers:::toolGetEUcountries b6c7b29d
44      toolInterpolateAndExtrapolate tool  mrdrivers      mrdrivers:::toolInterpolateAndExtrapolate c25d7871
45          toolHarmonizeFutureGrPast tool  mrdrivers          mrdrivers:::toolHarmonizeFutureGrPast 35a6744c
46                  toolHarmonizePast tool  mrdrivers                  mrdrivers:::toolHarmonizePast c52e64c8
47                     toolExtend2150 tool  mrdrivers                     mrdrivers:::toolExtend2150 37f53dbe
48                toolBezierExtension tool  mrdrivers                mrdrivers:::toolBezierExtension 76533cfd
49                 toolForestRelocate tool mrlandcore                mrlandcore:::toolForestRelocate 58fa2512
50                   toolLPJmLVersion tool mrlandcore                  mrlandcore:::toolLPJmLVersion 98ed7c9d
51                  toolCoord2Isocell tool    mstools                    mstools:::toolCoord2Isocell 735f7b0d
52                         toolSmooth tool    mstools                           mstools:::toolSmooth 241304af
53             toolHarmonize2Baseline tool    mstools               mstools:::toolHarmonize2Baseline e1f1b05e
54         toolConv2CountryByCelltype tool    mstools           mstools:::toolConv2CountryByCelltype c9843547
55        toolGetMappingCoord2Country tool    mstools          mstools:::toolGetMappingCoord2Country 9e393cd0
56                    toolSum2Country tool    mstools                      mstools:::toolSum2Country a717121e

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

madrat: a riddle, wrapped in a mystery, inside an enigma.

Some observations:

  • madrat does not care about what is in getConfig('packages') in general when determining the madrat graph (and therefore fingerprints):
    Restarting R session...
    
    > library(madrat)
    Loading required package: magclass
    > madrat:::fingerprint('calcBiomassPrices', details = TRUE) |>
    +   attr('details') |>
    +   head()
    
    …/FAO_livestock_carcass_price_factor.csv 
                                  "2a8ff669" 
                       …/FAOelementShort.csv 
                                  "756801e8" 
                 …/FAOiso_faocode_online.csv 
                                  "e3ec2252" 
         …/LPJ_CellBelongingsToCountries.csv 
                                  "58e5c571" 
                    UNKNOWN:::toolConvertGDP 
                                          NA 
                      madrat:::toolAggregate 
                                  "7783f250"
                                           …
    > getConfig('packages')
    [1] "madrat"                                         
    
  • It does care somewhat
                    UNKNOWN:::toolConvertGDP 
                                          NA 
    
  • While getMadratGraph() does not care
    > madrat:::fingerprint('calcFAOLand')
    [1] "f092f2e2"
    attr(,"call")
    [1] "mrfaocore:::calcFAOLand"
    
    calcOutput() does
    > calcOutput('FAOLand')
    
    Run calcOutput(type = "FAOLand")
    Error in prepFunctionName(type = type, prefix = "calc", ignore = ifelse(is.null(years),  : 
      Type "FAOLand" is not a valid output type. Check getCalculations("calc") for available types!
    Exit calcOutput(type = "FAOLand") in 0 seconds
    
  • All those packages get loaded – including GDPuc – but not attached
    > grep('^(m[rs]|GDPuc)', loadedNamespaces(), value = TRUE)
     [1] "mrfactors"       "mrdrivers"       "mrsoil"          "mrvalidation"    "mrland"
     [6] "mrremind"        "mrcommons"       "mstools"         "GDPuc"           "mredgebuildings"
    [11] "mrwater"         "mrmagpie"        "mrlandcore"      "mrtransport"     "mrfaocore"
    
    I have no idea why madrat finds the mr-packages, but not GDPuc
  • I cannot replicate your result.
  • I have no idea what causes
    ~ Run calcOutput(type = "FAOLand", aggregate = FALSE)
    ~~  - forced cache 
    
  • I need a madrat break. I will test some more next week.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

For madrat fingerprinting not all functions, see pik-piam/madrat#223.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

I tried again.

$ diff -y --suppress-common-lines <( idrpv rev6.99-poc-A 2>/dev/null ) <( idrpv rev6.99-poc-B 2>/dev/null )
Using /p/projects/rd3mod/inputdata/output_1.27/rev6.99-poc-A_ | Using /p/projects/rd3mod/inputdata/output_1.27/rev6.99-poc-B_
mrremind        0.193.9                                       | mrremind        0.194.0001
$ diff -I '^\*' -q ~pehl/swap/rev6.99-poc-A/ ~pehl/swap/rev6.99-poc-B/
Files /home/pehl/swap/rev6.99-poc-A/config.rds and /home/pehl/swap/rev6.99-poc-B/config.rds differ
Files /home/pehl/swap/rev6.99-poc-A/diagnostics.log and /home/pehl/swap/rev6.99-poc-B/diagnostics.log differ
Files /home/pehl/swap/rev6.99-poc-A/renv.lock and /home/pehl/swap/rev6.99-poc-B/renv.lock differ

Good to go.

@fbenke-pik fbenke-pik marked this pull request as ready for review October 24, 2024 10:06
@fbenke-pik fbenke-pik merged commit 9842bc0 into pik-piam:master Oct 24, 2024
2 checks passed
@fbenke-pik fbenke-pik deleted the poc branch October 24, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants