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

Fix PRICE_EMISSION definition #912

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jan 22, 2025

In preparation of the 3.10 release, @khaeru and I noticed that the original PR to fix PRICE_EMISSION (address #723) has become a little cluttered with discussions of how to create a proper test for the calculation update. Meanwhile, @volker-krey suggested that we should just bring the fix to main and open a follow-up issue to create the test later. Thus, this PR adds all changes from #726 that seem to be necessary to fix the calculation and make the tests pass as they are.

Caution

The error with the current calculation of PRICE_EMISSION went undetected so long because our tests were not stringent enough. Hence our initial insistence that #726 contain the exact kind of test to prevent the issue from coming up again. Such a test is missing from this PR on purpose, but that makes it incomplete in that sense. We need to maintain the sense of urgency that this test should be added as quickly as possible.
The only reason we are considering merging this without the test is because both @OFR-IIASA and @yiyi1991 manually confirmed that this fix works on sufficiently complex scenarios.

How to review

  • Read the diff and note that the CI checks all pass.
  • Ensure that changes/additions are self-documenting, i.e. that another
    developer (someone like the reviewer) will be able to understand what the code
    does in the future.
  • If you know of further places in our docs where the PRICE_EMISSION calculation is explained and needs updating, please share that.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update release notes.

@glatterf42 glatterf42 added bug Doesn't work as advertised/unintended effects safe to test labels Jan 22, 2025
@glatterf42 glatterf42 added this to the 3.10 milestone Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.6%. Comparing base (6fd03a8) to head (1c69bfd).
Report is 6 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #912   +/-   ##
=====================================
  Coverage   95.6%   95.6%           
=====================================
  Files         48      48           
  Lines       4394    4401    +7     
=====================================
+ Hits        4204    4211    +7     
  Misses       190     190           
Files with missing lines Coverage Δ
message_ix/models.py 99.0% <ø> (ø)
message_ix/tests/test_feature_price_emission.py 100.0% <100.0%> (ø)
message_ix/tests/test_report.py 100.0% <100.0%> (ø)
message_ix/tests/test_tutorials.py 97.6% <ø> (ø)

@glatterf42 glatterf42 self-assigned this Jan 22, 2025
@glatterf42 glatterf42 requested a review from khaeru January 22, 2025 12:24
@glatterf42 glatterf42 force-pushed the fix/price-emission-definition branch 2 times, most recently from 12c9110 to c5a39c3 Compare January 30, 2025 08:07
@khaeru khaeru force-pushed the fix/price-emission-definition branch from c5a39c3 to 0a8d658 Compare February 3, 2025 08:59
@khaeru
Copy link
Member

khaeru commented Feb 3, 2025

Thanks for excerpting a clean version of these changes.

  • Update release notes.

I will join to try to write an appropriate summary that alerts users to the nature of the change and its expected impact.

Per the commit messages, I'm not sure yet that adopting “conventional commits”—either optionally or as mandatory for all contributions—has benefit/cost > 1.

Can we either:

  • Modify this text to explain what alternate format is accepted
    - For both **commit messages** and **pull request (PR) titles**, memorize and use the `“7 rules of a great Git commit message” <https://chris.beams.io/posts/git-commit/#seven-rules>`_.
  • or, rewrite the messages for now?

khaeru added a commit to glatterf42/message_ix that referenced this pull request Feb 3, 2025
glatterf42 pushed a commit to glatterf42/message_ix that referenced this pull request Feb 3, 2025
@glatterf42 glatterf42 force-pushed the fix/price-emission-definition branch from 4ae7842 to f7a82d1 Compare February 3, 2025 12:28
@khaeru khaeru force-pushed the fix/price-emission-definition branch from f7a82d1 to 1c69bfd Compare February 3, 2025 13:54
@khaeru khaeru merged commit 37850eb into iiasa:main Feb 3, 2025
25 checks passed
@glatterf42 glatterf42 deleted the fix/price-emission-definition branch February 3, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Doesn't work as advertised/unintended effects safe to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants