Add FrozenVector to wrap neighbors

[simonschoelly]

This PR makes the output of neighbors, inneighbors and outneighbors immutable by wrapping the returned Vector in an immutable wrapper called FrozenVector.

I noticed that the generated machine code for Base.getindex is slightly different (one additional instruction) with FrozenVector compared to the previously used Vector - this does probably not have a significant performance impact, but I should run some benchmarks.

See also this issue: #316

[codecov]

Codecov Report

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

Comparison is base (cc3052f) 97.26% compared to head (8286e57) 97.23%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/frozenvector.jl	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
- Coverage   97.26%   97.23%   -0.03%     
==========================================
  Files         115      116       +1     
  Lines        6795     6805      +10     
==========================================
+ Hits         6609     6617       +8     
- Misses        186      188       +2     

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

[gdalle]

We could wait until #315 is merged to have a proper benchmarking pipeline?

[simonschoelly]

We could wait until #315 is merged to have a proper benchmarking pipeline?

That one has been merged now.

[gdalle]

It has been merged to a branch of the main repo (benchx) so that @filchristou could experiment with CI setups and actions outside of a fork. It's not yet on master, and more importantly even once the pipeline is set up we still won't have a satisfying set of tasks to benchmark.

Besides performance issues, I am personally opposed to this change, because the additional wrapper might be confusing for users (what is this FrozenVector thing and what can I do with it?) and incompletely implemented (what if we forget a useful vector operation?). Also I think there is a case for considering such a change breaking

[simonschoelly]

It has been merged to a branch of the main repo (benchx) so that @filchristou could experiment with CI setups and actions outside of a fork. It's not yet on master, and more importantly even once the pipeline is set up we still won't have a satisfying set of tasks to benchmark.

Right, I had just looked at the merged status.

Besides performance issues, I am personally opposed to this change, because the additional wrapper might be confusing for users (what is this FrozenVector thing and what can I do with it?) and incompletely implemented (what if we forget a useful vector operation?). Also I think there is a case for considering such a change breaking

Nowhere do we guarantee that we run an Vector. From the documentation:

  inneighbors(g, v)

  Return a list of all neighbors connected to vertex v by an incoming edge.

  Implementation Notes
  ––––––––––––––––––––

  Returns a reference to the current graph's internal structures, not a copy. Do not modify result. If the
  graph is modified, the behavior is undefined: the array behind this reference may be modified too, but
  this is not guaranteed.

And FrozenVector is even an AbstractVector although we also do not guarantee that by the documentation. So why should one use any other feature of a Vector and why should we support that?
On the other hand, we are able to prevent users from making a lot of hard to detect errors when they - for some -reason - try to modify that result without making a copy.

And I don't think performance should be affected seriously.

[simonschoelly]

Futhermore, if someone is actually confused what a FrozenVector is, they can simply consult the documentation.

[gdalle]

Okay, I'm convinced that the benefits outweigh the costs on the correctness side.

Can you maybe run some benchmarks locally, say Dijkstra on a large enough graph, to see if there is a big performance hit from creating the wrappers every time? Maybe even degree(g) could suffer a bit.

[filchristou]

I will probably merge to master before the community call. After you can use that for benchmarks and talk further.

[gdalle]

Awesome, thank you so much! Don't hesitate to ask for a review beforehand though, to follow ColPrac