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

add_year() as implemented results in scenario errors #578

Open
gidden opened this issue Apr 6, 2022 · 6 comments
Open

add_year() as implemented results in scenario errors #578

gidden opened this issue Apr 6, 2022 · 6 comments
Labels
awaiting info Needs more information from the issuer to continue bug Doesn't work as advertised/unintended effects

Comments

@gidden
Copy link
Member

gidden commented Apr 6, 2022

Observation: Using add_year() on the current main branch results in scenarios with misaligned input, output and technical_lifetime parameters. This can result in completely unexpected results. Below, for example, is the result of a high-carbon-tax scenario applied to an existing 5-year timestep model and the same model after using add_year() to add 1-year timesteps in the first 5-year period.

(this is an updated image originally published in a comment below but is relevant for new readers of this issue)
image

There is a believed to be correctly working version on this branch.

Immediate solution: Raise a NotImplementedError in main's implementation

Eventual solution: Migrate the correctly-working version into the main branch.

@gidden
Copy link
Member Author

gidden commented Apr 6, 2022

#579 implements an immediate solution

@gidden
Copy link
Member Author

gidden commented Apr 6, 2022

@behnam-zakeri and @khaeru can you please outline the steps necessary to implement the eventual solution?

@khaeru
Copy link
Member

khaeru commented Apr 6, 2022

There is a believed to be correctly working version on this branch.

@khaeru can you please outline the steps necessary to implement the eventual solution?

That branch (behnam-zakeri:test-add-year) seems to be related to iiasa:test-add-year, which I created for #494. As the title indicates, that PR was to improve test coverage and performance. It was not to address bugs (such as the one described here) because, at the time, we weren't aware of any. The PR stalled because the intended behaviour for some combinations of arguments was not clear, and I could not find time to thoroughly discuss it with @behnam-zakeri so that I could write the tests.

Because all that was unrelated to any bug, I don't know what the cause of the present bug is, and thus no, I can not say how it should be fixed without further investigation or details.

P.S. I also don't know whether, or how, the branch in the fork differs from the one in this repo.

@khaeru khaeru added bug Doesn't work as advertised/unintended effects awaiting info Needs more information from the issuer to continue labels Apr 6, 2022
@khaeru
Copy link
Member

khaeru commented Apr 6, 2022

results in misaligned input, output and technical_lifetime parameters

What specifically characterizes "aligned"? How specifically are data in the resulting scenarios "misaligned"?

believed to be correctly working version

What informs this belief? Does the branch code result in parameters (those mentioned above) aligned an the expected way, or is it simply that certain large scenarios produce expected outputs for some reported quantities?

Added the "awaiting info" label because this information is necessary to write tests, whether on #494 or elsewhere.

@gidden
Copy link
Member Author

gidden commented Apr 7, 2022

To follow up here, and any further clarification would be useful from @OFR-IIASA if needed.

What specifically characterizes "aligned"? How specifically are data in the resulting scenarios "misaligned"?

The data is misaligned in the sense that technoeconomic parameters (e.g., input, output) exist in the 1-year generated model beyond the time horizon implied for a vintage's technical_lifetime. There is no check against this in the formulation, so a technology can continue to operate as long as it has these technoeconomic parameters active.

What informs this belief?

Apart from personal discussions, the version on that branch was applied in a different study and appears based on a simple smoke test to perform as expected.

image

Here a 5-year version of a model (light blue) aligns with the 1-year version of that same model (yellow line) in a high-carbon tax scenario.

For a different version of the model, the 5-year version results as one might expect (orange line). The current version of add_year() on main results in strong deviations from expectation (blue line). The current version of add_year() on the cited branch results in slightly weaker deviations from expectation (grey line).

This implies that there are certain technologies/updates in the second model version than the first which result in the behavior seen, however it is still unexpected behavior.

@behnam-zakeri
Copy link
Contributor

behnam-zakeri commented Apr 28, 2022

Thanks @gidden and @khaeru for the notes, and sorry for the delay; I'm catching up after return from the leave. There are a few points here that I mention:

  1. When add_year() was initially developed early 2018, this was discussed in the weekly meeting (sorry, I cannot find the notes) that this feature should work without cross-checking the generated data with technical_lifetime. My impression was that this feature is not supposed to check and correct the possible inconsistency between technical_lifetime and input/output parameters in a given scenario, as it may change the behaviour (as discussed here Incorrect definition of map_tec_lifetime due to parametrization requirements of technical_lifetime #583). The feature should only add new years by interpolation/extrapolation of available data.
  2. The initial development worked until a project that we needed to add 1-year periods. Then, due to the code design that the minimum interval and lifetime was considered to be 5 years, things didn't work; and the branch test-add-year suggested improvements which resolved those issues. However, this was never brought back as a PR, due to the experimental nature of changes. After the end of the project, there was a misunderstanding that these changes were merged but they were not.
  3. As suggested by Test & improve perfomance in .tools.add_year #494 the code in add_year() needs to be improved using built-in function of python and methods of message_ix developed later. I will be happy to explain what the code is doing or supposed to do, if anyone with a better coding style can rewrite or improve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting info Needs more information from the issuer to continue bug Doesn't work as advertised/unintended effects
Projects
None yet
Development

No branches or pull requests

3 participants