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

Adding value to where in toolGetMapping #405

Merged
merged 8 commits into from
Jul 3, 2023

Conversation

whitehacker
Copy link
Contributor

Went through all warnings displayed by reposearch toolGetMapping in mrremind and adjusted the where value.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (2eed46a) 0.00% compared to head (51ab581) 0.00%.

❗ Current head 51ab581 differs from pull request most recent head 7e18f48. Consider uploading reports for the commit 7e18f48 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #405      +/-   ##
=========================================
- Coverage    0.00%   0.00%   -0.01%     
=========================================
  Files         266     266              
  Lines       17109   17152      +43     
=========================================
  Hits            1       1              
- Misses      17108   17151      +43     
Impacted Files Coverage Δ
R/calcAGEB.R 0.00% <0.00%> (ø)
R/calcBP.R 0.00% <0.00%> (ø)
R/calcCapTarget.R 0.00% <0.00%> (ø)
R/calcCapacityFactor.R 0.00% <0.00%> (ø)
R/calcCapital.R 0.00% <0.00%> (ø)
R/calcCementShare.R 0.00% <0.00%> (ø)
R/calcChemicalFeedstocksShare.R 0.00% <0.00%> (ø)
R/calcCoolingSharesBase.R 0.00% <0.00%> (ø)
R/calcCoolingSharesFuture.R 0.00% <0.00%> (ø)
R/calcEEAGHGProjections.R 0.00% <0.00%> (ø)
... and 56 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LaviniaBaumstark
Copy link
Member

Did you try that input data generation is still working? Did you compare old and new input data and ensured that they are the same?

R/calcAGEB.R Outdated
@@ -15,7 +15,7 @@
calcAGEB <- function(subtype = "balances") {
ageb <- readSource("AGEB", subtype = subtype)

mapping <- toolGetMapping("Mapping_AGEB_REMIND.csv", type = "reportingVariables") %>%
mapping <- toolGetMapping("Mapping_AGEB_REMIND.csv", type = "reportingVariables", where="mappingfolder") %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure that your added code does not produce linter warnings. This addition should cause a complaint about missing whitespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whitehacker
Copy link
Contributor Author

whitehacker commented Jun 21, 2023

Did you try that input data generation is still working? Did you compare old and new input data and ensured that they are the same?

Yes,
New Data: REMIND: /p/projects/rd3mod/inputdata/output/rev6.54baseerInputData_2b1450bc_remind.tgz
MAgPIE: /p/projects/rd3mod/inputdata/output/rev4.87baseerMP_h12_fd712c0b_cellularmagpie_c200_MRI-ESM2-0-ssp370_lpjml-8e6c5eb1.tgz

Comparison: inputdata-comparedata /p/projects/rd3mod/inputdata/output/rev6.54baseerInputData_2b1450bc_remind.tgz /p/projects/rd3mod/inputdata/output/rev6.54falks-cache_2b1450bc_remind.tgz

@LaviniaBaumstark
Copy link
Member

did you check that you are not generating linter warnings?

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

> invisible(system('git fetch --multiple origin baseer-github', intern = TRUE))
> invisible(system('git checkout -f baseer-github/master', intern = TRUE))
HEAD is now at dc7c6dd Fixed the error in R/calcEmissionFactors.R
> 
> suppressMessages(lint_new <- lintr::lint_package(linters = lucode2::lintrRules()))
> 
> invisible(system('git checkout -f origin/master', intern = TRUE))
Previous HEAD position was dc7c6dd Fixed the error in R/calcEmissionFactors.R
HEAD is now at 00c7fb9 Merge pull request #404 from orichters/NDC2023
> 
> suppressMessages(lint_old <- lintr::lint_package(linters = lucode2::lintrRules()))
> 
> c(length(lint_old), length(lint_new), length(lint_new) - length(lint_old))
[1] 23451 23492    41

A 0.2 % increase.

@fbenke-pik fbenke-pik self-requested a review July 3, 2023 09:07
@whitehacker whitehacker merged commit 623de95 into pik-piam:master Jul 3, 2023
1 check passed
@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

Not what I would have expected under a "fixed lintr warnings" commit, and somewhat obfuscating the commit history.

$ git diff --ignore-all-space -U0 c09aea9~ -- R/read*
diff --git a/R/readExpertGuess.R b/R/readExpertGuess.R
index 04b18a4..9057ff8 100644
--- a/R/readExpertGuess.R
+++ b/R/readExpertGuess.R
@@ -40 +40 @@ readExpertGuess <- function(subtype) {
-    a <- read.csv("co2prices.csv", sep = ";")
+    a <- read.csv("co2prices-2023-06.csv", sep = ";")
diff --git a/R/readUNFCCC_NDC.R b/R/readUNFCCC_NDC.R
index b9fa3ed..9a2850c 100644
--- a/R/readUNFCCC_NDC.R
+++ b/R/readUNFCCC_NDC.R
@@ -21 +21 @@ readUNFCCC_NDC <- function(subtype) {
-    NDCfile <- "NDC_2023-02-24.xlsx"
+    NDCfile <- "NDC_2023-05-31.xlsx"

Would have appreciated a rebase here.

@fbenke-pik
Copy link
Contributor

fbenke-pik commented Jul 10, 2023

None of the two changes are part of this PR. Am I missing something?
7021b1c
3da7cfc

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

They very much are, because c09aea9 does not just "fix lintr warnings" but also merges upstream changes. Like I said, obfuscating what is going on.

$ git log --graph --pretty='oneline' --abbrev-commit 42e7190..HEAD
*   623de95 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #405 from whitehacker/master
|\  
| * 7e18f48 Fixed lintr warnings
| *   c09aea9 Fixed lintr warnings
| |\  
| |/  
|/|   
* |   2eed46a Merge pull request #406 from orichters/co2prices
|\ \  
| * | 16b1597 buildLibrary
| * | 3da7cfc in mrremind/R/calcCO2Prices.R add to description that 2020 CO2 prices are added
|/ /  
* |   00c7fb9 Merge pull request #404 from orichters/NDC2023
|\ \  
| * | 7021b1c update NDC targets to 2023-05-31
|/ /  
| * 4b7da3d Fixed lintr warnings
| * dc7c6dd (baseer-github/master) Fixed the error in R/calcEmissionFactors.R
| * 16631c6 spacing out the equal sign in where = mappingfolder
| * fc72983 (fix/all_hail_the_lintr_god) Added v(mappingfolder)alue to w )re in toolGetMapping
| *   5789989 Merge branch 'master' of github.com:pik-piam/mrremind
| |\  
| |/  
|/|   
* |   fc8e2e8 Merge pull request #403 from mellamoSimon/calcEmissionFactorsFeedstocks
|\ \  
| * | 130b433 update version number
| * | e164907 bugfix emi factors for chem sector process emissions (zeros in USA)
|/ /  
* | 0136254 Merge pull request #401 from fbenke-pik/unfccc
* | 1be9ad1 increment version
* | 9cf3039 support reading in UNFCCC variables that cannot be referenced by fixed position
* | d1e248f remove UNFCCC variables that cannot be accurately parsed with the current logic
* | 695c099 Merge branch 'master' into unfccc
* | 3ca16fa (falk-personal/unfccc) Merge remote-tracking branch 'upstream/master' into unfccc
* | 8575f73 read UNFCCC data from 2023
 /  
* 11b64db Added value to whe(mappingfolder)re in toolGetMapping

@fbenke-pik
Copy link
Contributor

fbenke-pik commented Jul 10, 2023

Ok, I see. Not good.
@whitehacker What happened here?
@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q Is it worth trying to fix this? Best without rewriting history? If so, do you already have an idea how, or should I look into this?

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Jul 10, 2023

Ok, I see. Not good.

But not terrible, either. Just inconvenient.

Is it worth trying to fix this?

No.

Best without rewriting history?

You cannot change the history of a public repository without calling down the ire of the git gods upon yourself.

Clarification: the problem is that the commit history looks wonky. So "fixing the problem" would be by definition sacrilegious. But it is a "avoid this in the future" problem.

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.

4 participants