Skip to content
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

Test same node ID for different node types #1262

Closed
visr opened this issue Mar 14, 2024 · 6 comments · Fixed by #1330
Closed

Test same node ID for different node types #1262

visr opened this issue Mar 14, 2024 · 6 comments · Fixed by #1330
Labels
bug Indicates an unexpected problem or unintended behavior test Relates to unit testing

Comments

@visr
Copy link
Member

visr commented Mar 14, 2024

With #1113 we theoretically support having e.g. both Basin #5 and Pump #5 in the same model. Now that #1110 is merged, we should test that this works properly.

Related to #1256.

@visr visr added the test Relates to unit testing label Mar 14, 2024
@SouthEndMusic
Copy link
Collaborator

SouthEndMusic commented Mar 22, 2024

I suggest adapting the main_network_with_subnetworks model for this as it covers a lot of ground.

A Question about a crashing simulation by Josje van Houwelingen (does she have a Github account to tag? EDIT: @JvanHouwelingen) also pointed out that the sparse capacity matrix (see mainly allocation_init.jl) uses node ID numbers as indices directly, which isn't valid anymore already since the introduction of the MetaGraph. Would a dictionary be a good replacement here, or is there some sparse matrix object which allows generic index sets?

@visr visr added the bug Indicates an unexpected problem or unintended behavior label Mar 25, 2024
@visr
Copy link
Member Author

visr commented Mar 25, 2024

would a dictionary be a good replacement here, or is there some sparse matrix object which allows generic index sets?

It's worth checking if we can use this:

https://jump.dev/JuMP.jl/stable/manual/containers/#SparseAxisArray
https://jump.dev/JuMP.jl/stable/api/JuMP.Containers/#JuMP.Containers.SparseAxisArray

visr added a commit that referenced this issue Mar 25, 2024
Hofer-Julian pushed a commit that referenced this issue Mar 25, 2024
Alleviates #1262

This code will need to be removed again when that is fixed.
@SouthEndMusic
Copy link
Collaborator

would a dictionary be a good replacement here, or is there some sparse matrix object which allows generic index sets?

It's worth checking if we can use this:

https://jump.dev/JuMP.jl/stable/manual/containers/#SparseAxisArray https://jump.dev/JuMP.jl/stable/api/JuMP.Containers/#JuMP.Containers.SparseAxisArray

Why is this worth using? What is useful about the SparseAxisArray wrapper around a dictionary that cannot be achieved by just using a dictionary? Especially since there is no direct interaction between the capacity object and the other JuMP objects in our code.

@SouthEndMusic
Copy link
Collaborator

@visr I also have to make a get method which calls get on the wrapped dictionary, because accessing missing values is not supported on SparseAxisArray.

@visr
Copy link
Member Author

visr commented Mar 27, 2024

Oh yeah I just assumed it may be easier if it supports the array interface, but if that isn't the case, a dictionary is simpler.

@SouthEndMusic
Copy link
Collaborator

I realized that I don't actually need that get method after all, see 0229364. Keeping the use of the array interface in allocation_init.jl is nice 👍

@visr visr closed this as completed in #1330 Apr 4, 2024
visr pushed a commit that referenced this issue Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior test Relates to unit testing
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants