Fix caching behaviors: #1070, #1072, #1074 #1073
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Performance Results
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## psy4 #1073 +/- ##
==========================================
- Coverage 80.48% 80.38% -0.10%
==========================================
Files 115 115
Lines 12403 12379 -24
==========================================
- Hits 9982 9951 -31
- Misses 2421 2428 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| res::SimulationProblemResults{DecisionModelSimulationResults}, | ||
| result_keys, | ||
| timestamps::Vector{Dates.DateTime}, | ||
| store::Nothing, | ||
| store::Union{Nothing, <:SimulationStore}, |
There was a problem hiding this comment.
This weird interface relates to #1018 and could be fixed in this PR. @daniel-thom we discussed not doing this anymore.
There was a problem hiding this comment.
I'm not sure exactly what the proposal is here. For now I'm just mimicking the other _read_results method and passing the store straight through
There was a problem hiding this comment.
This is an interface issue @daniel-thom discussed where we pass a store to read from or nothing when nothing signifies that it is an in-memory store which makes the code confusing since it isn't obvious.
There was a problem hiding this comment.
So you'd like me to resolve the store to the in-memory store or not somewhere upstream so there's always a valid store getting passed around?
There was a problem hiding this comment.
I’m not sure that I would change this, at least not here. nothing also means that the user didn’t pass a store.
There was a problem hiding this comment.
what does not passing a store to get result mean? I don't think that is right because there is always a store for a simulation
There was a problem hiding this comment.
We could easily be talking around each other. There are many nuances here. The user can do either of these:
read_variable(results, “ActivePowerVariable__ThermalStandard”)
open_store(HdfSimulationStore, simulation_store_path, "r") do store
read_variable(results, “ActivePowerVariable__ThermalStandard”; store=store)
end
In the first case we have to get the store, which could in memory or h5. If it’s h5, we have to open it. It’s in-memory, we can just use it. There is one slightly awkward case one line 288.
There was a problem hiding this comment.
no one does this
open_store(HdfSimulationStore, simulation_store_path, "r") do store
read_variable(results, “ActivePowerVariable__ThermalStandard”; store=store)
end
Assuming this patter will happen make the code way less understandable for developers.
The most common way to access results is calling read_variables or read_realized_variables
There was a problem hiding this comment.
In the absence of consensus, are we okay keeping store::Union{Nothing, <:SimulationStore} in this non-user-facing method for the moment?
There was a problem hiding this comment.
I’ll propose a patch in a separate PR.
|
we should put this in the PSY4 branch to avoid massive rebases later. This improvement can wait until the PSY 4 release. |
|
Now fixes #1074 |
GabrielKS
changed the title
| if _are_results_cached(res, result_keys, timestamps, keys(cached_results)) | ||
| @debug "reading results from SimulationsResults cache" | ||
| @debug "reading results from SimulationsResults cache" # NOTE tests match on this |
There was a problem hiding this comment.
As a developer I find this useful. It calls my attention to it.
| else | ||
| @debug "reading results from data store" | ||
| @debug "reading results from data store" # NOTE tests match on this |
This fixes #1070 and #1072.
On #1070:
There is some inherent overhead to creating large arrays that cannot be avoided, but now that we use the cache the time for repeated calls to
_read_results(Matrix{Float64}...on my big dataset has decreased 30x from 0.09s to 0.003s:This is much better and closes the issue but is still unnecessarily heavy-handed for my PowerAnalytics component-wise reads, so later I may seek to add an option to PowerSimulations to perform
read_results_with_keysfor only one component at a time.On #1072:
Ended up being more refactoring than initially planned, but the upshot is that for the
DecisionModelSimulationResultsmethod ofload_results!the documentation now precisely describes what happens, what happens is good, the bug preventing it from loading more timestamps is gone, some code duplication has been removed, and there are lots more tests. I did not significantly edit theEmulationModelSimulationResultsmethod ofload_results!; it appears to have a significantly different interface that does not even take in time steps.