beacon-biosignals / LegolasFlux.jl

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

update to Legolas v0.5 #21

Closed ericphanson closed 1 year ago

ericphanson commented 1 year ago

Breakingness is debatable, re- https://github.com/beacon-biosignals/LegolasFlux.jl/issues/20

I think these are all the changes needed? Not 100% sure though. Tests pass at least.

closes #20

ericphanson commented 1 year ago

Maybe we should test that an old table can still be loaded. Could save out the DigitsModel from the example on main, check it in as a test, and then make sure we can load it here?

kleinschmidt commented 1 year ago

Maybe we should test that an old table can still be loaded. Could save out the DigitsModel from the example on main, check it in as a test, and then make sure we can load it here?

was goign to suggest exactly this.

I think it's also good to use the digitsmodel as a test for extensibility

codecov[bot] commented 1 year ago

Codecov Report

Merging #21 (41553f8) into main (e3fd92e) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #21   +/-   ##
=======================================
  Coverage   93.87%   93.87%           
=======================================
  Files           2        2           
  Lines          49       49           
=======================================
  Hits           46       46           
  Misses          3        3           
Impacted Files Coverage Δ
src/LegolasFlux.jl 91.17% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kleinschmidt commented 1 year ago

I think it's also good to use the digitsmodel as a test for extensibility

OIC this is included in teh tests, very good

ericphanson commented 1 year ago

Should that go in some Legolas guidance, or here?

kleinschmidt commented 1 year ago

Hm, good question. I think there are some specific points that are particularly relevant for upgrading schemas intended to be used like the Model extensions here, so I think it's worth calling them out in as concise a way as possible, since at the moment it's a bit extract those actionable things from teh legolas documentation/tour/various PR discussions. I do think a general legolas upgrade checklist would be good to include in legolas but not that's probably a slightly larger thing

ericphanson commented 1 year ago

I agree the current set of

teh legolas documentation/tour/various PR discussions

is not conducive to quickly learning how to upgrade, but I think we should PR docs there specifically, since none of the points you raised seem LegolasFlux specific. I'll merge this, then make that PR (which will use some of this PR as an example), then make another PR here linking out to those docs.

edit: https://github.com/beacon-biosignals/Legolas.jl/pull/71 & https://github.com/beacon-biosignals/LegolasFlux.jl/pull/23