JLD2 Integration

[AayushSabharwal]

#496

Uses JLD2's custom serialization functionality (https://juliaio.github.io/JLD2.jl/dev/customserialization/)

[AayushSabharwal]

Looking at https://github.com/JuliaGraphs/MetaGraphs.jl/blob/master/src/persistence.jl and from some testing I did, it appears as though GraphSpace.graph can be saved through JLD2.

Additionally, reimplementing all schedulers as function-like objects would make serialization significantly simpler. Currently, since ABM isn't mutable, model.scheduler = ... isn't possible. JLD2 when deserializing SerializedABM to ABM would (as of now) set the scheduler to fastest. The only other way would be manually converting ABM to SerializedABM (through a constructor) and give that to JLD2 to save, so that the reverse conversion function can take the scheduler as a parameter.

This issue also then crops up with ContinuousSpace.update_vel!. Here, the most viable options would be to:

• Save the name of the function as a Symbol
• Convert the entire ABM hierarchy to it's serializable state before sending it to JLD2 without using the custom serialization functionality (also solves the scheduler issue)

[Libbum]

I wonder if using accessors.jl would be good for this. I've had success with it setting model.rng to a new RandomDevice(). Will be a good discussion for tomorrow.

[AayushSabharwal]

I'll cover what I've written in tests before moving on to tackling OSMSpace.

I think the to_serializable, from_serializable API should be documented too, as a sort of replacement for JLD2's JLD2.wconvert and JLD2.rconvert if keywords need to be passed

[AayushSabharwal]

Is it an acceptable pattern to encapsulate repetitive tests in a function, or does that reduce traceability in the case that those tests fail? For example,

function check_model_properties(model, other)
    @test model.scheduler == other.scheduler
    @test model.rng == other.rng
    @test model.maxid.x == other.maxid.x
end

[AayushSabharwal]

There's some weird behaviour with comparing GridSpace.hoods, which I plan on looking into. Comparing the structs doesn't work, but comparing their fields passes

[Datseris]

I'll cover what I've written in tests before moving on to tackling OSMSpace.

I propose to do this in a different PR. OSMSpace is a bit "special" so to speak

[Datseris]

Is it an acceptable pattern to encapsulate repetitive tests in a function, or does that reduce traceability in the case that those tests fail?

From my end this is fine, I do something similar in DynamicalSystems.jl

[codecov-commenter]

Codecov Report

Merging #505 (d5bd7c5) into master (07115c7) will increase coverage by 0.51%.
The diff coverage is 97.43%.

@@            Coverage Diff             @@
##           master     #505      +/-   ##
==========================================
+ Coverage   93.04%   93.55%   +0.51%     
==========================================
  Files          20       21       +1     
  Lines        1265     1304      +39     
==========================================
+ Hits         1177     1220      +43     
+ Misses         88       84       -4     

Impacted Files	Coverage Δ
src/submodules/io/csv_integration.jl	100.00% <ø> (ø)
src/submodules/pathfinding/grid_pathfinder.jl	80.90% <0.00%> (+3.63%)	⬆️
src/submodules/io/jld2_integration.jl	100.00% <100.00%> (ø)
src/spaces/openstreetmap.jl	91.85% <0.00%> (+0.03%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07115c7...d5bd7c5. Read the comment docs.

[AayushSabharwal]

All that's left to test now is serialization of multi-agent models. 325 tests racked up already just for JLD2 (was ~3k at one point, while I was testing GridSpace.s by having a @test macro in a loop over eachindex)

[AayushSabharwal]

I moved daisyworld's scheduler outside of the initialization function, so it's accessible while deserializing during tests. Is this fine, or should it remain local?

[Libbum]

Sure, that's not a requirement to be internal as far as I remember.

[AayushSabharwal]

I'm not fully happy with how dump_to_jld2's docstring is phrased. I'll work on improving it. Should it also specify what fields are not saved (space.s), or how they're saved (model.agents)?

[Libbum]

No real issues with implementation at all—looking good!!

There are a few suggestions to improve documentation and user interactions, but most are minor. We can perhaps work through some of the doc fixes tomorrow in the meeting.

[AayushSabharwal]

It seems ../path wasn't the issue. Now it can't see the file. "schelling.jld2" seems to be the correct location of the file. The issue is still that JLD2 doesn't see SchellingAgent as a type while loading the file, so it creates its own reconstructed type which wreaks havoc.

[Libbum]

OK, so that's a bit more complicated. Is this a problem only in CI, only when building documentation, or is it the same locally?

[AayushSabharwal]

It's a problem when building docs, both in CI and locally

[AayushSabharwal]

I should add, I tried creating a file with a submodule, defining an agent and model, and save/loading it within the module. Didn't create any issues. For some reason it's only when building docs

[Libbum]

Alright. That'll take some investigation. I'll be out for the afternoon so don't know when I'll be able to focus on it.

[Libbum]

Following our weekly meeting, the decision has been made to drop the additions to the Schelling Example from this PR and merge the rest once the documentation is up to scratch. A new PR will be opened with the Schelling Example additions as-is, and we'll investigate the Documenter/JLD2 issue as time permits.

We've also decided to support OSM space by saving agent lat/long coordinates and expecting the user to know the map path to deserialize their model: we will rebuild the mapspace on-load.

[Libbum]

These doc changes look good. So the only outstanding thing is OSMSpace from my perspective.

[AayushSabharwal]

I'm removing hoods serialization, but apart from that OSMSpace is the only thing left. If I remember correctly, we planned to handle that in a separate PR

[Datseris]

yes go ahead and merge when tests pass but do not tag a new release. New releases should be tagged only when documentation exists. Feel free to open a tracking issue with the docs code pasted there.

[AayushSabharwal]

Ah. I just missed your comment.
I'll revert this commit then. The only docs missing is the example right?

[AayushSabharwal]

I think this branch can be kept for now? Since OSMSpace is coming up, along with the fix to schelling