Skip to content

Conversation

louispt1
Copy link
Contributor

@louispt1 louispt1 commented Aug 18, 2025

Fetching the curves this way speeds up the exports for pyetm by about a minute.

Goes with this pyetm PR

Copy link
Member

@noracato noracato left a comment

Choose a reason for hiding this comment

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

Feels a lot cleaner, nice work!

I left a few comments here and there, but they're just some nitpicks

# Returns a single CSV file with specified output curves as columns.
# Query parameter 'curve_types' specifies which curves to include (comma-separated).
def bulk_output_curves
curve_types = params[:curve_types]&.split(',')&.map(&:strip)
Copy link
Member

Choose a reason for hiding this comment

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

Let's do this with permit as []

Copy link
Member

Choose a reason for hiding this comment

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

Or even with a require? That way we can tackle an empty csv automagically before we move into the serializer

Comment on lines +6 to +9
def initialize(scenario, curve_types)
@scenario = scenario
@curve_types = curve_types || []
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def initialize(scenario, curve_types)
@scenario = scenario
@curve_types = curve_types || []
end
def initialize(scenario, curve_types = [])
@scenario = scenario
@curve_types = curve_types
end

end

max_len = curves.values.map(&:length).max || 0
require 'csv'
Copy link
Member

Choose a reason for hiding this comment

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

We can move this one to the top!

@@ -26,6 +26,7 @@
get :costs_parameters, to: 'export#costs_parameters'
get :sankey, to: 'export#sankey'
get :storage_parameters, to: 'export#storage_parameters'
get :bulk_output_curves, to: 'export#bulk_output_curves'
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking the endpoint is already called (output)curves. Maybe we can rename this to just bulk or combined for clarity? What do you think?

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.

2 participants