Create constraints with new TulipaConstraint object #928

abelsiqueira · 2024-11-14T20:57:15Z

Update highest resolution table computation to use union tables and move highest_... out of dataframes.
Create TulipaConstraint structure.
Instead of incoming and outgoing expressions, create a dictionary of expressions inside TulipaConstraint.
This is necessary at the moment because some additional expressions were being created in addition to incoming_flow and outgoing_flow.
Create TulipaConstraint objects for all other constraints, including the ones that are copies of variables.
Remove the dataframes variable completely.
Clean up add_..._constraints signatures and related places

Closes #927, #918, #895, #611

github-actions · 2024-11-14T20:57:34Z

Benchmark Results

	`127db68`...	`5f33cfc`...	`127db68`.../5f33cfc08c5a68...
energy_problem/create_model	47.4 ± 2.8 s	43 ± 1.8 s	1.1
energy_problem/input_and_constructor	15.3 ± 0.38 s	20.1 ± 0.24 s	0.763
time_to_load	3.99 ± 0.026 s	4.03 ± 0.054 s	0.991

	`127db68`...	`5f33cfc`...	`127db68`.../5f33cfc08c5a68...
energy_problem/create_model	0.656 G allocs: 21.5 GB	0.664 G allocs: 21.1 GB	1.02
energy_problem/input_and_constructor	0.0691 G allocs: 2.61 GB	0.0651 G allocs: 2.47 GB	1.06
time_to_load	0.159 k allocs: 11.2 kB	0.159 k allocs: 11.2 kB	1

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

codecov · 2024-11-14T21:06:54Z

Codecov Report

Attention: Patch coverage is 97.11538% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.76%. Comparing base (127db68) to head (5f33cfc).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/model-preparation.jl	91.42%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
- Coverage   96.17%   95.76%   -0.41%     
==========================================
  Files          30       30              
  Lines        1202     1110      -92     
==========================================
- Hits         1156     1063      -93     
- Misses         46       47       +1

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

Update highest resolution table computation to use union tables and move highest_... out of dataframes Create TulipaConstraint structure. Instead of incoming and outgoing expressions, create a dictionary of expressions inside TulipaConstraint. This is necessary at the moment because some additional expressions were being created in addition to incoming_flow and outgoing_flow. Create TulipaConstraint objects for all other constraints, including the ones that are copies of variables. Remove the dataframes variable completely. Clean up add_..._constraints signatures and related places

src/create-model.jl

datejada · 2024-12-02T11:53:18Z

src/model-preparation.jl

-        incoming_flow_storage_inter_rp_balance,
-        outgoing_flow_storage_inter_rp_balance,
-    )
+    return


should we be explicit about the return? I mean like this? I am unsure if it makes a difference

Suggested change

return

return nothing

It's just a coding style choice, it doesn't make a difference (https://docs.julialang.org/en/v1/manual/functions/#Returning-nothing). I don't think there is a JuliaFormatter option to force one or the other. There is an option to force writing return (https://domluna.github.io/JuliaFormatter.jl/dev/#always_use_return, #796 ), but it's something else.
If we do decide on anything, we should add it to https://github.com/TulipaEnergy/TulipaEnergyModel.jl/blob/main/docs/src/91-developer.md#code-format-and-guidelines

Thanks! I like enforcing the return, but I don't have a strong opinion on the nothing. So, I created #940 for this.

test/test-time-resolution.jl

datejada · 2024-12-02T12:40:13Z

src/tmp.jl

+    # Outgoing flows
+    DuckDB.execute(
+        connection,
+        "CREATE OR REPLACE TEMP TABLE t_union_out_flows AS


These new tables are available in the connection object. Having an overview of all tables available in the connection will be nice, including which ones are read from the files/DB/csv and the ones that are created internally in the model.

That's the Miro description. When finished it can be rewritten into an official SQL description if desired, to have something in code format.

Great, I have created #941 to keep it in our minds.

datejada · 2024-12-02T13:46:13Z

src/constraints/create.jl

+    constraints[:storage_level_intra_rp] = TulipaConstraint(
+        DuckDB.query(
+            connection,
+            "SELECT * FROM storage_level_intra_rp
+            ",
+        ) |> DataFrame,
+    )
+
+    constraints[:storage_level_inter_rp] = TulipaConstraint(
+        DuckDB.query(
+            connection,
+            "SELECT * FROM storage_level_inter_rp
+            ",
+        ) |> DataFrame,
+    )


This is because the both the constraint and the variable use the same created table in the connection. Is that correct?

Yes, that's correct

gnawin · 2024-12-02T15:22:16Z

src/tmp.jl

+    # Incoming flows
+    DuckDB.execute(
+        connection,
+        "CREATE OR REPLACE TEMP TABLE t_union_in_flows AS


Is it okay to just use CREATE?

It should be, in the final version we can change to that, if preferred. However, for development it makes it easier to rerun some functions without rerunning everything. If only CREATE is used and the table exists, it will fail and then you have to manually delete the table or create a new connection object.

gnawin · 2024-12-02T15:22:46Z

src/tmp.jl

+    DuckDB.execute(
+        connection,
+        "CREATE OR REPLACE TEMP TABLE t_union_in_flows AS
+        SELECT DISTINCT to_asset as asset, year, rep_period, time_block_start, time_block_end


Also here, okay to use only "SELECT"? Do we have duplicates?

For this table it is ok to write only SELECT because they are filtered later in the resolution computation, but might as well do it now.
There are probably duplicates because this ignores from_asset.

gnawin · 2024-12-02T15:28:40Z

src/tmp.jl

+        SELECT DISTINCT asset, year, rep_period, time_block_start, time_block_end
+        FROM asset_time_resolution
+        UNION
+        FROM t_union_out_flows


Sorry I'm also learning SQL a bit in the process: is it fine to neglect SELECT after UNION :)

Not for all SQL languages, but DuckDB allows writing FROM table without the SELECT. It means SELECT * FROM table. We can be explicit, if preferred

gnawin · 2024-12-02T15:37:54Z

src/constraints/create.jl

+function compute_constraints_indices(connection)
+    constraints = Dict{Symbol,TulipaConstraint}()
+
+    constraints[:lowest] = TulipaConstraint(


I'm checking miro at the same time, there we have lowest_allflows, lowest_all? Which one is this, and better naming (or for later)?

ah, this seems to be an in-between step that is not shown on miro...so it takes t_lowest_all_flows (line 17) and then filter

This still uses the old table names. Miro shows the tables after renaming and some splitting, which hasn't happened yet to avoid further complications. There are some purple stickers in the bottom of the constraint tables showing where they come from.

abelsiqueira · 2024-12-03T13:39:17Z

@gnawin, @datejada, I've replied to all comments and made another commit fixing one of the issues

datejada

Thanks for the replies and updates. I created a couple of issues to work on after we have all the tables on MIRO in the code and a final overview of them.

abelsiqueira added the benchmark PR only - Run benchmark on PR label Nov 14, 2024

abelsiqueira force-pushed the 927-tulipa-constraints branch from 56f019f to 17eab49 Compare November 25, 2024 17:55

abelsiqueira changed the title ~~927 tulipa constraints~~ Create constraints with new TulipaConstraint object Nov 25, 2024

Remove file time-resolution

e5a3fbd

abelsiqueira marked this pull request as ready for review November 25, 2024 18:34

abelsiqueira requested review from gnawin and datejada November 25, 2024 18:34

datejada reviewed Dec 2, 2024

View reviewed changes

src/create-model.jl Outdated Show resolved Hide resolved

datejada reviewed Dec 2, 2024

View reviewed changes

test/test-time-resolution.jl Outdated Show resolved Hide resolved

datejada reviewed Dec 2, 2024

View reviewed changes

gnawin reviewed Dec 2, 2024

View reviewed changes

Rename add_expressions_to_dataframe

83a87db

abelsiqueira requested review from gnawin and datejada December 3, 2024 13:39

Better document the lowest resolution computation

5f33cfc

abelsiqueira mentioned this pull request Dec 3, 2024

Remove *_partitions from graph object #939

Merged

This was referenced Dec 4, 2024

Update JuliaFormater options #940

Open

Add a description of the tables available in the connection for the docs #941

Open

datejada approved these changes Dec 4, 2024

View reviewed changes

abelsiqueira merged commit f7440b9 into main Dec 4, 2024
5 of 7 checks passed

abelsiqueira deleted the 927-tulipa-constraints branch December 4, 2024 09:14

This was referenced Dec 4, 2024

Create constraints indices for units_on and units_on_and_outflows #918

Closed

Define partition tables #895

Closed

Change construct_dataframes to use tables (a.k.a. DuckDB) or input data frames #611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create constraints with new TulipaConstraint object #928

Create constraints with new TulipaConstraint object #928

abelsiqueira commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

datejada Dec 2, 2024

abelsiqueira Dec 3, 2024

datejada Dec 4, 2024

datejada Dec 2, 2024

abelsiqueira Dec 3, 2024

datejada Dec 4, 2024

datejada Dec 2, 2024

abelsiqueira Dec 3, 2024

gnawin Dec 2, 2024

abelsiqueira Dec 3, 2024

gnawin Dec 2, 2024

abelsiqueira Dec 3, 2024

gnawin Dec 2, 2024

abelsiqueira Dec 3, 2024

gnawin Dec 2, 2024

gnawin Dec 2, 2024

abelsiqueira Dec 3, 2024

abelsiqueira commented Dec 3, 2024

datejada left a comment

Create constraints with new TulipaConstraint object #928

Create constraints with new TulipaConstraint object #928

Conversation

abelsiqueira commented Nov 14, 2024 • edited Loading

github-actions bot commented Nov 14, 2024 • edited Loading

Benchmark Results

Benchmark Plots

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelsiqueira commented Dec 3, 2024

datejada left a comment

Choose a reason for hiding this comment

abelsiqueira commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading