Add Historical Industry Value Added Figures to historical.mif #396

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-12T12:38:41Z

Test runs pending …

fbenke-pik · 2023-05-12T13:39:10Z

Not sure if putting read and convert into one file is a problem (UNIDO.R as opposed to creating two files readUNIDO.R and convertUNIDO.R) @LaviniaBaumstark can you comment on this?

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-12T13:52:25Z

Not sure if putting read and convert into one file is a problem (UNIDO.R as opposed to creating two files readUNIDO.R and convertUNIDO.R) @LaviniaBaumstark can you comment on this?

Thought experiment: pick a random R function (not from within the pik-piam sphere) and tell me the name of the source file it was is.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-15T06:49:41Z

No unexpected differences in output.

$ diff -I '^\*' -q rev6.52_62eff8f7_remind/ rev6.52-test-UNIDO_62eff8f7_remind/ | column -t
Files  rev6.52_62eff8f7_remind/config.rds       and  rev6.52-test-UNIDO_62eff8f7_remind/config.rds       differ
Files  rev6.52_62eff8f7_remind/diagnostics.log  and  rev6.52-test-UNIDO_62eff8f7_remind/diagnostics.log  differ
Files  rev6.52_62eff8f7_remind/pucFiles         and  rev6.52-test-UNIDO_62eff8f7_remind/pucFiles         differ

$ diff -I '^\*' -q rev6.52_62eff8f7_validationremind/ rev6.52-test-UNIDO_62eff8f7_validationremind/ | column -t
Files  rev6.52_62eff8f7_validationremind/config.rds       and  rev6.52-test-UNIDO_62eff8f7_validationremind/config.rds       differ
Files  rev6.52_62eff8f7_validationremind/diagnostics.log  and  rev6.52-test-UNIDO_62eff8f7_validationremind/diagnostics.log  differ
Files  rev6.52_62eff8f7_validationremind/historical.mif   and  rev6.52-test-UNIDO_62eff8f7_validationremind/historical.mif   differ
Files  rev6.52_62eff8f7_validationremind/pucFiles         and  rev6.52-test-UNIDO_62eff8f7_validationremind/pucFiles         differ

$ diff rev6.52_62eff8f7_validationremind/historical.mif rev6.52-test-UNIDO_62eff8f7_validationremind/historical.mif | cut -c -80
10576a10577,10628
> INDSTAT2;historical;CAZ;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;CHA;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;EUR;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;IND;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;JPN;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;LAM;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;MEA;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;NEU;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;OAS;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;REF;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;SSA;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;USA;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;World;cement;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;CAZ;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;CHA;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;EUR;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;IND;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;JPN;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;LAM;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;MEA;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;NEU;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;OAS;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;REF;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;SSA;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;USA;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;World;chemicals;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/
> INDSTAT2;historical;CAZ;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;CHA;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;EUR;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;IND;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;JPN;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;LAM;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;MEA;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;NEU;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;OAS;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;REF;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;SSA;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;USA;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N
> INDSTAT2;historical;World;otherInd;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A
> INDSTAT2;historical;CAZ;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;CHA;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;EUR;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;IND;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;JPN;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;LAM;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;MEA;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;NEU;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;OAS;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;REF;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;SSA;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;USA;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;
> INDSTAT2;historical;World;steel;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/A;N/

LaviniaBaumstark

please, split UNIDO.R into readUNIDO.R and ConvertUNIDO.R, otherwise some functionality of madrat functions will get lost

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-15T09:01:11Z

please, split UNIDO.R into readUNIDO.R and ConvertUNIDO.R, otherwise some functionality of madrat functions will get lost

LaviniaBaumstark · 2023-05-15T09:09:37Z

Also if most of the functionality also works for your version, the rule is different and all madrat function rely on this. I cannot guarantee (and also not test) that all madrat functionality is working for your version. If everyone follows the rules, clear interfaces are provided. In addition, following the rules enables other developer to understand your code easier.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-15T09:22:21Z

Also if most of the functionality also works for your version, the rule is different and all madrat function rely on this. I cannot guarantee (and also not test) that all madrat functionality is working for your version. If everyone follows the rules, clear interfaces are provided. In addition, following the rules enables other developer to understand your code easier.

There is no functionality in madrat that depends on either the name of source code files, or the number of functions defined inside source code files. There can't be, because that information is simply not available to the madrat package.

LaviniaBaumstark · 2023-05-15T10:21:30Z

why do you want to make it different?

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-15T12:16:22Z

Because readUNIDO(), convertUNIDO(), calcUNIDO() are a logical unit. Working on any of those functions is easier when one has an understanding what the preceding function does. Thus it is helpful to have them in one file.

fbenke-pik · 2023-05-15T13:07:52Z

We checked back with the rest of the RSE group. Michaja, you are right, there is no technical requirement to write separate functions.

Splitting read and convert into functions with the same name is the recommended way to do it, but not a convention we enforce. We recommend it as a convention, because it helps newcomers with finding specific functionality fast and getting a quick overview of what the R functions of an mr package do.

So if you insist on keeping it that way, this won't block this PR.

R/calcHistorical.R

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q · 2023-05-15T14:22:24Z

We recommend it as a convention, because it helps newcomers with finding specific functionality fast and getting a quick overview of what the R functions of an mr package do.

Not sure whether "finding specific functionality fast" is the right term for scrolling through a list of 267 files whose names start all with one of three words …
Honestly, teach newcomers to search for code instead.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q requested a review from fbenke-pik May 12, 2023 12:38

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q force-pushed the dev/add_hist_ind_VA branch from 69af07e to 442f5ff Compare May 12, 2023 15:05

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q marked this pull request as ready for review May 15, 2023 06:50

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q force-pushed the dev/add_hist_ind_VA branch from 442f5ff to de92737 Compare May 15, 2023 07:38

LaviniaBaumstark requested changes May 15, 2023

View reviewed changes

fbenke-pik requested changes May 15, 2023

View reviewed changes

R/calcHistorical.R Outdated Show resolved Hide resolved

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q force-pushed the dev/add_hist_ind_VA branch from de92737 to 8bee547 Compare May 15, 2023 14:18

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q requested a review from fbenke-pik May 15, 2023 14:22

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q mentioned this pull request May 15, 2023

Dev/datarise full validationremind #397

Closed

fbenke-pik approved these changes May 15, 2023

View reviewed changes

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q requested a review from LaviniaBaumstark May 16, 2023 07:42

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q mentioned this pull request May 16, 2023

False negative on R CMD check via buildLibrary() on Github Workflow pik-piam/lucode2#170

Closed

LaviniaBaumstark approved these changes May 16, 2023

View reviewed changes

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q added 4 commits May 16, 2023 10:59

add convertUNIDO()

7bf78d1

add calcUNIDO()

98b4084

add industry value added calculation to fullVALIDATIONREMIND()

b84c18a

lucode hubbub

fa1dcb8

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q force-pushed the dev/add_hist_ind_VA branch from 8bee547 to fa1dcb8 Compare May 16, 2023 09:16

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q merged commit e6d3fdb into pik-piam:master May 16, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q deleted the dev/add_hist_ind_VA branch May 16, 2023 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Historical Industry Value Added Figures to historical.mif #396

Add Historical Industry Value Added Figures to historical.mif #396

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 12, 2023

fbenke-pik commented May 12, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 12, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

LaviniaBaumstark left a comment

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

LaviniaBaumstark commented May 15, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

LaviniaBaumstark commented May 15, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

fbenke-pik commented May 15, 2023 •

edited

Loading

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

Add Historical Industry Value Added Figures to historical.mif #396

Add Historical Industry Value Added Figures to historical.mif #396

Conversation

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 12, 2023

fbenke-pik commented May 12, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 12, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

LaviniaBaumstark left a comment

Choose a reason for hiding this comment

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

LaviniaBaumstark commented May 15, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

LaviniaBaumstark commented May 15, 2023

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

fbenke-pik commented May 15, 2023 • edited Loading

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented May 15, 2023

fbenke-pik commented May 15, 2023 •

edited

Loading