Skip to content

Change indices function in the fixed_om_costs function #1123

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

Conversation

datejada
Copy link
Member

@datejada datejada commented Oct 29, 2024

Using units_invested_available_indices in the function fixed_om_costs to avoid errors when using the representative periods feature.

Fixes #1122

Checklist before merging

  • Documentation is up-to-date
  • Unit tests have been added/updated accordingly
  • Code has been formatted according to SpineOpt's style
  • Unit tests pass

@datejada
Copy link
Member Author

@nnhjy I am putting back the indices of the function fixed_om_costs as units_invested_available_indices rather than unit_flow_indices as you changed in #1117. I am not sure why you changed the indices, but there must be a reason. Would you mind reviewing that what you want to achieve in your changes still works with these indices? If your changes work with this change, we should keep the indices as they were before so they work correctly with the representative periods feature.
Thanks!

@nnhjy
Copy link
Member

nnhjy commented Oct 29, 2024

Thanks, @datejada! This reminds me of why units_invested_available_indices was used. (Probably it was also me who made it so the fom_cost could work with some cases with the representative period feature.) Now, the whole picture is clearer.

As mentioned in #1117, using units_invested_available_indices would overlook the non-invested units, i.e. those units defined simply by number_of unit. That was the very reason I changed the setting.

There needs a solution to cover both settings for a unit while allowing for the the representative. I can give a try to fix that if you don't mind working with this spontaneous branch this week.

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (0263576) to head (4c02a70).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1123   +/-   ##
=======================================
  Coverage   87.33%   87.33%           
=======================================
  Files         143      143           
  Lines        4137     4137           
=======================================
  Hits         3613     3613           
  Misses        524      524           

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

@datejada datejada marked this pull request as ready for review October 29, 2024 09:57
@datejada
Copy link
Member Author

Thanks, @datejada! This reminds me of why units_invested_available_indices was used. (Probably it was also me who made it so the fom_cost could work with some cases with the representative period feature.) Now, the whole picture is clearer.

As mentioned in #1117, using units_invested_available_indices would overlook the non-invested units, i.e. those units defined simply by number_of unit. That was the very reason I changed the setting.

There needs a solution to cover both settings for a unit while allowing for the the representative. I can give a try to fix that if you don't mind working with this spontaneous branch this week.

@nnhjy great! Thanks for the quick reply. You have the full picture 😄 so, it would be much appreciated if you could look at it. Please let me know if I can help you with this.

@nnhjy
Copy link
Member

nnhjy commented Oct 30, 2024

@datejada Would the latest commits resolve issue #1122 ?

@datejada
Copy link
Member Author

@datejada Would the latest commits resolve issue #1122 ?

Hi @nnhjy, yes, it does; thank you so much.

@datejada datejada merged commit 7bc3fcf into master Oct 31, 2024
7 checks passed
@datejada datejada deleted the 1122-bug-function-fixed_om_costs-throws-and-error-when-using-representative-periods branch October 31, 2024 08:14
@nnhjy nnhjy restored the 1122-bug-function-fixed_om_costs-throws-and-error-when-using-representative-periods branch November 4, 2024 08:08
nnhjy added a commit that referenced this pull request Nov 4, 2024
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.

[bug] function fixed_om_costs throws and error when using representative periods
2 participants