invenia / ObservationDims.jl

Traits for specifying the orientation of features and observations in data
MIT License
2 stars 1 forks source link

Remove direct dependency on AxisArrays #27

Closed glennmoy closed 3 years ago

glennmoy commented 3 years ago

Related to https://github.com/invenia/ObservationDims.jl/issues/3

Draft status as I want to discuss the below first and then implement the same solution for NamedDims.

There are 3 ways we can do this:

  1. Keep things as they are - we don't seem to be adding new types too often, but it means downstream dependencies implicitly depend on AxisArrays.jl. This is no longer the ideal as we want to remove AxisArrays as a dependency in our internal packages.
  2. Implicitly support AxisArrays / NamedDims by way of a hacky "duck-typing" (this PR). This is non-breaking and removes the dependency, but it makes the code more complicated than it needs to be and doesn't fully satisfy https://github.com/invenia/ObservationDims.jl/issues/3.
  3. Let AxisArrays / NamedDims.jl directly support ObservationDims.jl as https://github.com/invenia/ObservationDims.jl/issues/3 suggests. This is probably easier to do now than it previously was since we're dropping AxisArrays anyway, and it's easier for us to make the necessary changes to NamedDims.jl (and AxisKeys.jl if we wanted).
codecov[bot] commented 3 years ago

Codecov Report

Merging #27 (b535fdf) into master (2f1ce6c) will decrease coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   98.52%   98.50%   -0.03%     
==========================================
  Files           1        1              
  Lines          68       67       -1     
==========================================
- Hits           67       66       -1     
  Misses          1        1              
Impacted Files Coverage Δ
src/ObservationDims.jl 98.50% <100.00%> (-0.03%) :arrow_down:

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 2f1ce6c...b535fdf. Read the comment docs.

glennmoy commented 3 years ago

@oxinabox do you have any opinion on the various options in the PR description? and if you think I should do the same for NamedDims while I'm at it?

oxinabox commented 3 years ago

I do not have strong ones. I think what is done here is acceptable. I do not think you should do the same for NamedDims. AxisArrays is a almost deprecated package, supporting it poorly via hacks feels ok.

I am not in any particular hurry to invert the dependencies. We are only using this package in one place AFAIK. (or is it used in FeatureTransforms.jl?) If it turns out to be a good idea then we will use it in more and then it might be worth inverting the dependencies. But that is for a later day