beacon-biosignals / LegolasFlux.jl

Save Flux model weights in Legolas-powered Arrow tables
MIT License
6 stars 1 forks source link

Only extract numeric arrays #5

Closed haberdashPI closed 3 years ago

haberdashPI commented 3 years ago

LegolasFlux was assuming that any array had parameters to save. In the model I'm working on, I use arrays to store some of the layers (maybe this is bad practice?), which leads to the fetch_weights having an element that is an array of Chain objects. This breaks a call to write the model out to an arrow table.

Here, I've addressed this by filtering on all arrays that contain elements that are numbers, consistent with the assumptions of the serialization methods.

codecov[bot] commented 3 years ago

Codecov Report

Merging #5 (621489c) into main (cd965c8) will increase coverage by 0.31%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
+ Coverage   94.23%   94.54%   +0.31%     
==========================================
  Files           2        2              
  Lines          52       55       +3     
==========================================
+ Hits           49       52       +3     
  Misses          3        3              
Impacted Files Coverage Δ
src/functors.jl 100.00% <100.00%> (ø)

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 cd965c8...621489c. Read the comment docs.

ericphanson commented 3 years ago

I use arrays to store some of the layers (maybe this is bad practice?)

I think it depends on how many layers you have and if they are all the same type. Chain uses tuples which (as you know) are good for heterogeneous types, and encode the size in the type (and are not good for huge objects like 100s of layers), while arrays are good for homogenous types and large number of objects (though even if they are all the same type, if you only have a small number of objects, a tuple might be a better choice, since the compiler can use the fact that it knows the length of the tuple at compile time). So I'd likely use a chain with chains inside it instead of a vector of chains.

Anyways I think the choice between vector and tuple should be an optimization rather than give you different semantics (which is why this is an important bugfix). Though I'm not totally sure how the larger Flux ecosystem treats these things.