1D container bugs

[luke-kiernan]

For uniform interface reasons, we should avoid one dimensional containers, so that we can safely index as x[i, j]. Look into the codebase to make sure anywhere we're creating containers, they're 2 dimensional: a matrix of size (n, 1), not a vector.

edit: the bug that inspired this issue is PSI issue #76

edit^2: PR on similar issue, PSI #1518

[luke-kiernan]

The decided upon long-term fix is that we should check the dimensions upon creation, make sure it's 2D not 1D.

[yasirroni]

Hi, @luke-kiernan.

Based on StorageSystemsSimulations.jl/src/storage_models.jl, both StorageEnergyShortageVariable and StorageEnergySurplusVariable is only binding at time_steps[end].

    for d in devices
        name = PSY.get_name(d)
        target = PSY.get_storage_target(d)
        constraint_container[name] = JuMP.@constraint(
            PSI.get_jump_model(container),
            energy_var[name, time_steps[end]] - surplus_var[name] + shortfall_var[name] == target
        )

...

function PSI.add_variables!(
    container::PSI.OptimizationContainer,
    ::Type{T},
    devices::IS.FlattenIteratorWrapper{U},
    formulation::AbstractStorageFormulation,
) where {
    T <: Union{StorageEnergyShortageVariable, StorageEnergySurplusVariable},
    U <: PSY.Storage,
}
    @assert !isempty(devices)
    variable = PSI.add_variable_container!(container, T(), U, PSY.get_name.(devices))
    for d in devices
        name = PSY.get_name(d)
        variable[name] = JuMP.@variable(
            PSI.get_jump_model(container),
            base_name = "$(T)_{$(PSY.get_name(d))}",
            lower_bound = 0.0
        )
    end
    return
end

So, the solution is to refactor it into:

function PSI.add_variables!(
    container::PSI.OptimizationContainer,
    ::Type{T},
    devices::IS.FlattenIteratorWrapper{U},
    formulation::AbstractStorageFormulation,
) where {
    T <: Union{StorageEnergyShortageVariable, StorageEnergySurplusVariable},
    U <: PSY.Storage,
}
    @assert !isempty(devices)
    time_steps = PSI.get_time_steps(container)
    variable = PSI.add_variable_container!(container, T(), U, PSY.get_name.(devices), time_steps[end])
    for d in devices
        name = PSY.get_name(d)
        variable[name, time_steps[end]] = JuMP.@variable(
            PSI.get_jump_model(container),
            base_name = "$(T)_{$(PSY.get_name(d))}",
            lower_bound = 0.0
        )
    end
    return
end

Please correct me if I'm wrong.

[luke-kiernan]

How this related to the title of the issue: currently we have variable[name] = .... You're suggesting it ought to be variable[name, time_steps[end]] = ....

edit: enough issues are surfacing related to this that it seems worth addressing in PSI. My current thought is it just add something like

IS.@assert_op !(valtype(container) <: DenseArray) || ndims(valtype(container)) == 2

to _assign_container!, so that all dense containers must be 2D. If the PSI tests pass with that addition, I'll open up a PR over there.

[yasirroni]

You're suggesting it ought to be variable[name, time_steps[end]] = ....

It is more on what I'm understand based on your explanation of:

the dimensions upon creation, make sure it's 2D not 1D.

---

That is because make it into full variable[name, end] will make the simulation slowed due to big unused variable.

---

Otherwise, I'll follow any directions that all of you decided. I'll try to help as good as I can.

[jd-lara]

You're suggesting it ought to be variable[name, time_steps[end]] = ....

It is more on what I'm understand based on your explanation of:

the dimensions upon creation, make sure it's 2D not 1D.

That is because make it into full variable[name, end] will make the simulation slowed due to big unused variable.

Otherwise, I'll follow any directions that all of you decided. I'll try to help as good as I can.

The code in this repo won't be put in production until a few months from now, the fix in StorageSystemsSimulations.jl is the correct one