new internal API for sampling

[Tortar]

This will solve #837 when finished

[codecov-commenter]

Codecov Report

Attention: 71 lines in your changes are missing coverage. Please review.

Comparison is base (cbb8140) 85.71% compared to head (045752c) 69.84%.

❗ 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 ⚠️
src/core/space_interaction_API.jl	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #875       +/-   ##
===========================================
- Coverage   85.71%   69.84%   -15.88%     
===========================================
  Files          36       42        +6     
  Lines        2542     2792      +250     
===========================================
- Hits         2179     1950      -229     
- Misses        363      842      +479     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Tortar]

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

[Tortar]

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)

[Tortar]

actually there are some ensemble tests which didn't fail locally, not sure why at the moment

[Tortar]

mmmmh, they seem actually totally unrelated

[Datseris]

Have you checked for performance regressions? The introduction of Iteartors.Filter e.g. could add some.

[Tortar]

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 :-)

[Datseris]

yes, this is good from me as well, but all tests fail!

[Tortar]

I know but the tests that fail seem spurious, this doesn't happen locally, it happens also in #946 and #957

[Tortar]

If you could try to run tests too locally it would be even more solid to conclude that something is strange just on github

[Datseris]

If tests pass locally I trust you. We would need to figure out how to solve this before v6. (if tests fail on main after merging this, we should also open an issue to keep track of things.

[Tortar]

tests passed when you merged, so I think it alright