Skip to content

Conversation

@glatterf42
Copy link
Member

This PR supersedes iiasa/message-ix-models#197 as @khaeru suggested this tool might be better placed in message_ix.

@behnam-zakeri Please take a look at the changes. Several things come to mind before merging this:

  • Use the existing logging functionality instead of printing to stdout
  • Write a bit of documentation on how to use this tool (e.g. via a README.rst as was done for add_year)
  • Write tests for this new functionality.

How to review

Check the above boxes plus:

  • Read the diff and note that the CI checks all pass.
  • Run a specific code snippet or command and check the output.
  • Build the documentation and look at a certain page.
  • 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.

PR checklist

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

@codecov
Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 255 lines in your changes missing coverage. Please review.

Project coverage is 90.3%. Comparing base (1125579) to head (baeb7f8).
Report is 39 commits behind head on main.

Files with missing lines Patch % Lines
message_ix/tools/add_timeslice/add_timeslice.py 0.0% 255 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1125579) and HEAD (baeb7f8). Click for more details.

HEAD has 133 uploads less than BASE
Flag BASE (1125579) HEAD (baeb7f8)
151 18
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #852     +/-   ##
=======================================
- Coverage   95.6%   90.3%   -5.4%     
=======================================
  Files         46      47      +1     
  Lines       4333    4588    +255     
=======================================
  Hits        4143    4143             
- Misses       190     445    +255     
Files with missing lines Coverage Δ
message_ix/tools/add_timeslice/add_timeslice.py 0.0% <0.0%> (ø)

@glatterf42
Copy link
Member Author

In the MESSAGE meeting just now, @behnam-zakeri mentioned that #680 might be related.

@behnam-zakeri
Copy link
Contributor

behnam-zakeri commented Oct 24, 2024

@glatterf42 Related to the test, I have added an example of Westeros model via the Excel file, which is used by the code to create a Westeros scenario with time slices, exactly similar to the one we have in our published tutorials (Westeros_seasonality.ipynb). So, the only part missing will be to check if the output of these two models are the same or not.

  1. Running the new code with input data from Westeros_baseline scenario and config data from Excel. [done]
  2. Loading an already existing or running Westeros_Seasonality scenario (can be done easily).
  3. Comparing the results of 1 and 2, e.g.:
  • asserting if "time" related sets are the same ("time", "duration_time")
  • asserting OBJ.
  • asserting ACT of coal_ppl or wind_ppl in different times (winter or summer).

@glatterf42
Copy link
Member Author

Unfortunately, neither @behnam-zakeri nor me have enough time to finish this PR in time for the 3.10 milestone. We hope this will be better for 3.11, though.

@glatterf42 glatterf42 modified the milestones: 3.10, 3.11 Jan 15, 2025
@khaeru khaeru modified the milestones: 3.11, 3.12 May 27, 2025
@glatterf42 glatterf42 removed their assignment Nov 28, 2025
@glatterf42
Copy link
Member Author

glatterf42 commented Nov 28, 2025

Not a lot seems to be missing here (mainly tests), but I don't think I'll have time to finish this before my leave. So I'm not sure when this will finally be nudged over the finish line.

@khaeru
Copy link
Member

khaeru commented Dec 8, 2025

I'm not sure when this will finally be nudged over the finish line.

Let's set this as assigned to no-one: if/when there is a concrete decision about who will work on completing it then we can adjust the status again.

@khaeru khaeru removed this from the 3.12 milestone Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enh New features & functionality timeslice

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants