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

Correct logic of Scenario.years_active() #281

Merged
merged 13 commits into from Dec 12, 2019
Merged

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Dec 11, 2019

Closes #278.
Closes #279.

  • Scenario.years_active() and .vintage_and_active_years() are improved.
  • test_years_active() is rewritten to be more explicit.
  • test_years_active() was incorrect. In this PR, the period '2050' is removed from the expected return value. See the inline comments:
    # technical_lifetime of seattle/canning_plant/2020 is 30 years.
    # - constructed in 2011-01-01.
    # - by 2020-12-31, has operated 10 years.
    # - operates until 2040-12-31.
    # - is NOT active within the period '2050' (2041-01-01 to 2050-12-31)
    result = scen.years_active('seattle', 'canning_plant', '2020')
    npt.assert_array_equal(result, years[1:-1])

I have followed our documentation, specifically here where it reads:

In MESSAGEix, the key of an element in set year identifies the last year of the period, i.e., in a set year = [2000, 2005, 2010, 2015], the period ‘2010’ comprises the years [2006, .., 2010].

IMO this is more confusing than it needs to be, and we should rename 'year' to 'period' to disambiguate.

PR checklist:

  • Tests added.
  • Documentation added.
  • Release notes updated.

@khaeru khaeru changed the title Correct logic Correct logic of Scenario.years_active() Dec 11, 2019
@khaeru khaeru self-assigned this Dec 11, 2019
@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #281 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #281      +/-   ##
=========================================
+ Coverage   78.19%   78.2%   +0.01%     
=========================================
  Files          13      13              
  Lines        1142    1138       -4     
=========================================
- Hits          893     890       -3     
+ Misses        249     248       -1
Impacted Files Coverage Δ
message_ix/core.py 90.64% <100%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b86ae19...66d5b4d. Read the comment docs.

@OFR-IIASA
Copy link
Contributor

@khaeru I have managed to switch branches and this looks fine to me. Should we also resolve issue #279 with this PR?

Copy link
Contributor

@OFR-IIASA OFR-IIASA left a comment

Choose a reason for hiding this comment

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

Successfully tested.

@khaeru
Copy link
Member Author

khaeru commented Dec 12, 2019

Should we also resolve issue #279 with this PR?

Yes, I already did. See the description at the top, and the commit message for 5f6deae.

@khaeru
Copy link
Member Author

khaeru commented Dec 12, 2019

Note that the CI checks passed for cca6a25, then failed for 0022513. This seems to be the same symptom as noted here on #276.

@khaeru
Copy link
Member Author

khaeru commented Dec 12, 2019

Note that the CI checks passed for cca6a25, then failed for 0022513. This seems to be the same symptom as noted here on #276.

66d5b4d brings in the fix for this issue from master. If the checks pass with this change, I will merge.

@khaeru khaeru merged commit d7a75e6 into iiasa:master Dec 12, 2019
@khaeru
Copy link
Member Author

khaeru commented Dec 12, 2019

Total coding time on this, #278, #279, and #283: 2 hours 20 minutes.
Discussion, filing #282, monitoring CI: about 1:00.

@khaeru khaeru deleted the issue/278 branch December 12, 2019 12:55
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.

vintage_and_active_years() fails when there are no 'year_pairs' years_active() returns wrong years
2 participants