Closed Tortar closed 7 months ago
Attention: 71 lines
in your changes are missing coverage. Please review.
Comparison is base (
cbb8140
) 85.71% compared to head (045752c
) 69.84%.:exclamation: Current head 045752c differs from pull request most recent head 9cfc01c. Consider uploading reports for the commit 9cfc01c to get more accurate results
Files | Patch % | Lines |
---|---|---|
src/simulations/sample.jl | 33.96% | 70 Missing :warning: |
src/core/space_interaction_API.jl | 88.88% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This will actually be solved when I manage to find the time to finish the work in https://github.com/Tortar/IteratorSampling.jl
hopefully soon
Everything works now, so I think it should be good to go
I needed to add some more Pkg tricks though because I noticed that if you didn't have internet connection the solution in #955 didn't work (and so you couldn't test the package at all because it tried to fetch it from github), I verified that instead it works like this. This (mess :D) is only temporary, when we release v6 we will rapidly delete all of this.
I made our sampling depending from https://github.com/Tortar/IteratorSampling.jl where the sampling process is much more well tested than how it was in our library, so I guess it's okay to depend from it :-)
Anyway there we have also fast sampling for multiple items, but here I didn't include it because it requires some more work (issue tracked in #837)
actually there are some ensemble tests which didn't fail locally, not sure why at the moment
mmmmh, they seem actually totally unrelated
checked briefly with this code
using Agents, BenchmarkTools
@agent A GridAgent{2} begin
end
agent_step!(a,m) = nothing
function aperiodic_model(; griddims = (100, 100))
space = GridSpace(griddims, periodic = false)
model = ABM(A, space; agent_step!)
fill_space!(model)
return model
end
function periodic_model(; griddims = (100, 100))
space = GridSpace(griddims, periodic = true)
model = ABM(A, space; agent_step!)
fill_space!(model)
return model
end
@btime random_nearby_id($(1,1), model, 10) setup=(model=periodic_model());
@btime random_nearby_id($(50,50), model, 10) setup=(model=periodic_model());
@btime random_nearby_id($(1,1), model, 1) setup=(model=aperiodic_model());
@btime random_nearby_id($(50,50), model, 1) setup=(model=aperiodic_model());
no perf regression, same perf, in general I expect the same for all the other function apart from a little improvement where I introduced the new Iterators.filter
in random_empty_pos_in_offsets
because it is a fallback when there is a small number of empty cells, in this case using the other methodology (sampling_with_condition_single
function) is bad for performance.
So I think this PR should be good to go as it is. Let's see in the future if we want to support optimized multi sampling procedures or maybe give to the users some advice on how to perform them by themselves. Maybe we can discuss that in our call too :-)
yes, this is good from me as well, but all tests fail!
I know but the tests that fail seem spurious, this doesn't happen locally, it happens also in #946 and #957
If you could try to run tests too locally it would be even more solid to conclude that something is strange just on github
tests passed when you merged, so I think it alright
This will solve #837 when finished