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

Create model discount parameters #803

Closed
wants to merge 5 commits into from

Conversation

gnawin
Copy link
Member

@gnawin gnawin commented Sep 17, 2024

Pull request details

Describe the changes made in this pull request

  • create a toml file for data on the model level (should be scenario data in the future) only for multi-year
  • this file is directly read in create_model. Note this means all other cases will use this toml. The discount year will matter to other test cases. Since all other cases have a year of 2030, making a discount year of 2030 would mean doing nothing.
  • the generalization of this data and separation of model data and scenario data should be done in the refactor Refactor the model #701 (under AND THEN)

List of related issues or pull requests

Closes #802

Collaboration confirmation

As a contributor I confirm

  • I read and followed the instructions in README.dev.md
  • The documentation is up to date with the changes introduced in this Pull Request (or NA)
  • Tests are passing
  • Lint is passing

@gnawin
Copy link
Member Author

gnawin commented Sep 17, 2024

Hi both @abelsiqueira @datejada, I have this draft PR to hear your opinions.

The idea is to create two parameters that apply to the model, i.e., which do not depend on the assets. Since we currently don't have a place for that, I just put them in graph-assets-data. Do you think there is a better way to do it now?

For example, I thought about creating a new file. But since my references are the current files, which look differently from this potential file (just two names with two parameters), I don't yet see a straightforward way to do it.

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (21650c1) to head (bdc0915).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #803   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines          961       964    +3     
=========================================
+ Hits           961       964    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@datejada
Copy link
Member

Hi both @abelsiqueira @datejada, I have this draft PR to hear your opinions.

The idea is to create two parameters that apply to the model, i.e., which do not depend on the assets. Since we currently don't have a place for that, I just put them in graph-assets-data. Do you think there is a better way to do it now?

For example, I thought about creating a new file. But since my references are the current files, which look differently from this potential file (just two names with two parameters), I don't yet see a straightforward way to do it.

This proposal is not ideal, we will repeat values unnecessarily. I was looking for inspiration in other models and this data is normally stored in a new entity/file/table with the model properties/constants/parameters.

What is the best way to go forward in Tulipa? I am not sure, but I am leaning toward a separate file (CSV, TOML, YAML, or JSON) to store this information and any other constant that we might want to define or need in the model.

@abelsiqueira
Copy link
Member

I agree, this should be a separate file. I recommend using TOML:

model_discount_rate = 0.03
model_discount_year = 2020

Since these are parameters, not data, it makes sense that they have a different, simpler, format. Also, that they will be manage outside of TulipaIO.

@gnawin
Copy link
Member Author

gnawin commented Sep 18, 2024

Thank you both @datejada @abelsiqueira.

I created a toml file. Since it is outside TulipaIO, so I suppose it's not part of the connection (which currently is tied to read_csv in TulipaIO), but rather something else. For that I added an extra argument in io.jl. Note that this extra argument will need to added to lots of other places.

I just want to check with you if this is fine before I make changes accordingly.

@abelsiqueira
Copy link
Member

It is not part of the connection. Some questions before judging how to handle it:

  • Is the file mandatory? Do you need it in all uses of Tulipa? If you don't have one, is there a reasonable default for the parameters?
  • Where do you begin to need these parameters? Which function?
  • Is it part of the EnergyProblem? Does it make sense to keep these parameters inside it?

@gnawin
Copy link
Member Author

gnawin commented Sep 19, 2024

@abelsiqueira thanks for thinking along:

  • Is the file mandatory? Do you need it in all uses of Tulipa? If you don't have one, is there a reasonable default for the parameters?

This file (with the parameters I need now) is only useful for multi-year.

  • Where do you begin to need these parameters? Which function?

I need these in create_model, because they are needed in the objective function. Initially I thought about putting it directly there, but then I hesitated because I thought it should be part of io and EnergyProblem (see below answer).

  • Is it part of the EnergyProblem? Does it make sense to keep these parameters inside it?

Since these parameters would influence the results (objective function). I would think it is part of EnergyProblem.

That being all said, I think it is also fine if we put it somewhere with minimal effort, because I see this (as part of scenario data) would be probably reconsidered in the refactor.

@abelsiqueira
Copy link
Member

If it's planned as a temporary solution, I would just open the file directly in create_model! and then we discuss these questions in more details later.
Also, I would suggest not creating the file for every test/input, since it's only necessary for multi-year. This way you ensure you're not making it necessary.
Also, it's good to add to #701 to remember that we have to change it afterwards.

@gnawin gnawin marked this pull request as ready for review September 19, 2024 09:17
@gnawin gnawin mentioned this pull request Sep 19, 2024
52 tasks
@gnawin gnawin added this to the M3 - End Sept milestone Sep 19, 2024
@gnawin
Copy link
Member Author

gnawin commented Sep 19, 2024

@abelsiqueira I made the changes accordingly, but read in the data in create_model instead of create_model!, this way I don't have to add additional arguments for now.

@gnawin gnawin requested a review from abelsiqueira September 19, 2024 09:53
@gnawin gnawin removed this from the M3 - End Sept milestone Sep 19, 2024
Copy link
Member

@abelsiqueira abelsiqueira left a comment

Choose a reason for hiding this comment

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

That's too hard-coded. It's not conditional on the problem, so it is reading the parameters for all inputs, not only the multi-year. Furthermore, it is reading a fixed file, which is the same as explicitly defining the variables in the file (i.e., hard-code them).
My thinking was you give the file as argument to create_model. If nothing is passed, you don't open anything. You would still need to change a few places, but you don't carry anything around.

If you need a quick hack to continue other PRs, you might as well just explicitly define model_discount_rate and model_discount_year in the create_model! function. But then it might be more useful to do it in the specific PR, than do it here.

What is the goal of #802? Enable reading model parameters for general use? Or just create these two parameters somewhere? Or something else, maybe? What is it blocking?

@gnawin
Copy link
Member Author

gnawin commented Sep 20, 2024

Closed in favor of #813.

@gnawin gnawin closed this Sep 20, 2024
@gnawin gnawin deleted the 802-create-model-parameters branch September 20, 2024 12:30
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.

Create economic parameters
3 participants