fixing tabular parser #1126

claytonpbarrows · 2024-06-13T02:01:10Z

I've replicated the PSY3.0 objective function created by the tabular parsing of the RTS using the FuelCurve functionality. However, I haven't developed tests. In the interest of time, I'm opening this PR now.

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

JuliaFormatter

src/parsers/power_system_table_data.jl

jd-lara · 2024-06-13T14:50:55Z

@claytonpbarrows I changed the target branch to the doc strings changes PR so that we can avoid name clashed and @rodrigomha can test PSB with all the changes in the same place

GabrielKS

My general comment would be that I think the new cost structs can be taken further advantage of to reduce the amount of math that needs to happen here, representing the data in a form more similar to how it was given to us.

It'd be great to get some more robust testing for this — creating some simple input data, parsing them by hand, and comparing to what this code gives, rather than basing the testing on the correctness of the previous implementation. I'll let someone else decide whether we have time for that before the release, but if not it should at least get converted to an issue to be added in a minor version soon.

src/parsers/power_system_table_data.jl

GabrielKS · 2024-06-14T16:33:51Z

src/parsers/power_system_table_data.jl

+
+        if !in(nothing, [x, y])
+            push!(vals,
+                (x = tryparse(Float64, string(x)) * base_power,


Why are you multiplying by base_power rather than setting the units system of the cost/fuel curve to device base? If that would in fact represent the same data and this is being done just because because PowerSimulations needs it to be in natural units, maybe we should have some conversion code elsewhere to make this math more reusable? I'd be happy to implement that.

In the interest of getting the major release done, I'm okay keeping it as-is for now (assuming the as-is math is correct), then changing the implementation later.

src/parsers/power_system_table_data.jl

GabrielKS · 2024-06-14T16:49:51Z

src/parsers/power_system_table_data.jl

        # if there is only one point, use it to determine the constant $/MW cost
-        var_cost = LinearFunctionData(var_cost[1][1] * fuel_cost + vom)
-        fixed = 0.0
+        var_cost = LinearCurve(cost_pairs[1].y)


This matches what we had before, but what we had before seemed a little suspicious to me. Are we sure it's just y, not y/x or something?

A PiecewisePointCurve is an InputOutputCurve, so x is ~MW and y is $/hr. A LinearCurve is also an InputOutputCurve, so same thing. But when you construct a LinearCurve with only one argument, you're specifying the slope of that curve, which should therefore be $/MWh. So this is implying that the units of y are different when there's only one point.

Unless there's a special case in the input data spec for only one point (units are truly different, x is always 1, etc.), I would think the options for parsing one point are:

We want to create a flat input-output curve (constant $ / hr, as opposed to constant $/MWh), which would be LinearCurve(0.0, LinearCurve(cost_pairs[1].y))

We want to create a linear input-output curve going through the origin and the single point we have, which would be LinearCurve(cost_pairs[1].y/cost_pairs[1].x)

Adding to my suspicion is the fact that we're doing the same thing below in create_pwinc_cost, where the y values are supposed to have different units

The non-fuel option is more flexible since we don't really have data in that format. So, I agree with this point and made the suggested change.

Am I understanding correctly that we get to make up the spec here since there's no existing data we're trying to match? The implementation as it now stands seems at least internally consistent, though I wonder why we decided that cost curves should be input-output curves and fuel curves should be incremental. Is there a rationale for that?

GabrielKS · 2024-06-14T16:50:20Z

src/parsers/power_system_table_data.jl

-        fixed = 0.0
+        var_cost = LinearCurve(cost_pairs[1].y)
+    else
+        @warn "Unable to calculate variable cost for $(gen.name)" cost_pairs maxlog = 5


maybe specify that the reason for the failure is that there are zero points

src/parsers/power_system_table_data.jl

GabrielKS · 2024-06-14T17:07:03Z

src/parsers/power_system_table_data.jl

+    cost_colnames::_CostPointColumns,
+) where {T <: ThermalGen}
+    cost_pairs = get_cost_pairs(gen, cost_colnames)
+    var_cost = create_pwl_cost(cost_pairs)


Is it the case that when the input data is a fuel curve, it's given as incremental/marginal, whereas when the input data is a cost curve, it's given as input-output? That seems plausible, just making sure. If so, maybe put in a comment to that effect.

yes. the cost-point data option is less defined and less exercised.

(see above my question regarding why we decided to make cost curves input-output when fuel curves are incremental)

src/parsers/power_system_table_data.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions · 2024-06-24T18:22:33Z

src/parsers/power_system_table_data.jl

+            first(y_points) * first(x_points),
+            x_points,
+            y_points[2:end],
+            )


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

)

)

src/parsers/power_system_table_data.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jd-lara · 2024-06-24T22:25:18Z

@claytonpbarrows we will merge this to enable a better parsing in psy4 but as discussed with @GabrielKS, there needs to be one more step of cleaning up to be done with the Plexos2Sienna effort.

fixing tabular parser

53355ce

claytonpbarrows requested a review from GabrielKS June 13, 2024 02:01

github-actions bot reviewed Jun 13, 2024

View reviewed changes

whitespace

ee29416

jd-lara changed the base branch from psy4 to kdrh/psy4_docs_renaming June 13, 2024 14:50

jd-lara assigned GabrielKS Jun 13, 2024

jd-lara added PARSING issues related to parsing come New Version 4.0 labels Jun 13, 2024

GabrielKS requested changes Jun 14, 2024

View reviewed changes

fixing single cost point option

d81c50a

github-actions bot reviewed Jun 20, 2024

View reviewed changes

src/parsers/power_system_table_data.jl Outdated Show resolved Hide resolved

cleaning up tabular fuel cost parser

c18e9ba

github-actions bot reviewed Jun 24, 2024

View reviewed changes

src/parsers/power_system_table_data.jl Outdated Show resolved Hide resolved

src/parsers/power_system_table_data.jl Outdated Show resolved Hide resolved

jd-lara requested a review from GabrielKS June 24, 2024 18:02

claytonpbarrows and others added 2 commits June 24, 2024 12:21

Update src/parsers/power_system_table_data.jl

c8af4bc

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Update src/parsers/power_system_table_data.jl

e383e09

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot reviewed Jun 24, 2024

View reviewed changes

Update power_system_table_data.jl

e244473

github-actions bot reviewed Jun 24, 2024

View reviewed changes

src/parsers/power_system_table_data.jl Outdated Show resolved Hide resolved

Update src/parsers/power_system_table_data.jl

f689faa

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jd-lara merged commit 51d1443 into kdrh/psy4_docs_renaming Jun 24, 2024
1 of 8 checks passed

GabrielKS mentioned this pull request Jul 1, 2024

Kdrh/psy4 docs renaming #1141

Merged

jd-lara deleted the cb/psy4_tabular_parser branch July 1, 2024 23:53

GabrielKS mentioned this pull request Oct 14, 2024

feat: Adding capability to create PiecewiseLinear from csv parser #1202

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing tabular parser #1126

fixing tabular parser #1126

claytonpbarrows commented Jun 13, 2024

github-actions bot left a comment

jd-lara commented Jun 13, 2024

GabrielKS left a comment

GabrielKS Jun 14, 2024

GabrielKS Jun 24, 2024

GabrielKS Jun 14, 2024 •

edited

Loading

GabrielKS Jun 14, 2024

claytonpbarrows Jun 20, 2024

GabrielKS Jun 24, 2024

GabrielKS Jun 14, 2024

GabrielKS Jun 14, 2024

claytonpbarrows Jun 20, 2024

GabrielKS Jun 24, 2024

github-actions bot Jun 24, 2024

jd-lara commented Jun 24, 2024

fixing tabular parser #1126

fixing tabular parser #1126

Conversation

claytonpbarrows commented Jun 13, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

jd-lara commented Jun 13, 2024

GabrielKS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GabrielKS Jun 14, 2024 • edited Loading

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

github-actions bot Jun 24, 2024

Choose a reason for hiding this comment

jd-lara commented Jun 24, 2024

GabrielKS Jun 14, 2024 •

edited

Loading