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

Remove alternative 32_power realizations that are not used anymore #1769

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

lecfab
Copy link
Contributor

@lecfab lecfab commented Aug 6, 2024

Purpose of this PR

In module 32_power, only IntC is used (see conversation and corresponding issue).

Type of change

(Make sure to delete from the Type-of-change list the items not relevant to your PR)

  • Spring cleaning
  • Refactoring
  • New feature
  • Minor change (default scenarios show only small differences)
  • Fundamental change
  • This change requires a documentation update

Checklist:

  • My code follows the coding etiquette
  • I performed a self-review of my own code
  • I explained my changes within the PR, particularly in hard-to-understand areas
  • I checked that the in-code documentation is up-to-date
  • I adjusted the reporting in remind2 where it was needed
  • I adjusted forbiddenColumnNames in readCheckScenarioConfig.R in case the PR leads to deprecated switches
  • All automated model tests pass (FAIL 0 in the output of make test)
  • The changelog CHANGELOG.md has been updated correctly

Further information (optional):

  • Test runs are here: /p/tmp/fabricel/powerRealizations/output
  • Comparison of results (what changes by this PR?): nothing, just remove some realizations for 32_power

+ ( (2020 - t.val)/15 * fm_dataglob("learnMult_wFC",teLearn)
*** 2005 to 2020: linear transition from global 2005 to regional 2020
*** to phase-in the observed 2020 regional variation from input-data
+ ( (2020 - t.val) / (2020-2015) * fm_dataglob("learnMult_wFC",teLearn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you replace 15 by 2020-2015 = 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to hide some bugs for more fun in the future 🤷

It's a mistake thanks for digging and finding it!

@lecfab lecfab marked this pull request as ready for review August 8, 2024 11:34
Copy link
Contributor

@orichters orichters left a comment

Choose a reason for hiding this comment

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

It looks good from my side, although I still don't understand why this changes the testOneRegi behavior as noticed here. I would prefer if you could wait for @LaviniaBaumstark's approval before merging, maybe she has an idea.

Thanks for the clean-up.

@lecfab
Copy link
Contributor Author

lecfab commented Aug 8, 2024

I still don't understand why this changes the testOneRegi behavior as noticed here. I would prefer if you could wait for @LaviniaBaumstark's approval before merging, maybe she has an idea.

Yes let's do that!

@lecfab
Copy link
Contributor Author

lecfab commented Aug 27, 2024

Now that we understand why the issue with testOneRegi occurred, I think we can merge this PR.
Further cleaning is ongoing in a separate issue about removing demSeOth.

What do you think @orichters @fbenke-pik ?

@orichters
Copy link
Contributor

Fine from my side.

@lecfab lecfab merged commit 7153325 into remindmodel:develop Aug 27, 2024
2 checks passed
@lecfab lecfab deleted the powerRealizationsRebased branch August 27, 2024 13:38
Comment on lines -159 to +164
+ sum(prodSeOth2te(enty2,te), vm_prodSeOth(t,regi,enty2,te) )
+ sum(prodSeOth2te(enty2,te), v_prodSeOth(t,regi,enty2,te) ) !! *** RLDC removal
+ vm_Mport(t,regi,enty2)
=e=
sum(se2fe(enty2,enty3,te), vm_demSe(t,regi,enty2,enty3,te))
+ sum(se2se(enty2,enty3,te), vm_demSe(t,regi,enty2,enty3,te))
+ sum(demSeOth2te(enty2,te), vm_demSeOth(t,regi,enty2,te) )
+ sum(demSeOth2te(enty2,te), v_demSeOth(t,regi,enty2,te) ) !! *** RLDC removal

Choose a reason for hiding this comment

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

Is !! *** RLDC removal intended to conway any information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, they are anchors for remaining todos (related to this SeOth issue with which you helped me today)

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.

3 participants