Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate away from Dierckx.jl #54

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

icweaver
Copy link
Member

Addressing the AD concern brought up by Miles

AFAICT, Interpolations.jl still doesn't seem to have ootb support for cubic B-splines on irregular grids just yet

BSplineKit.jl seems like a very nice drop-in replacement though. The output differs slightly from Dierckx.jl, but still seems to be within our test tolerances. Might just be a boundary condition thing I'm doing wrong, but still need to explore more

DataInterpolations.jl could also be the way to go. Do folks have other thoughts?

@icweaver icweaver changed the title Migrate away from Dier Migrate away from Dierckx.jl Feb 13, 2025
@icweaver icweaver mentioned this pull request Feb 13, 2025
10 tasks
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (af42144) to head (29f3053).

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files          10       10              
  Lines         270      268       -2     
==========================================
- Hits          269      267       -2     
  Misses          1        1              

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

@cgarling
Copy link
Member

I've used DataInterpolations.jl when I had some data that their specialty interpolators were useful for. I don't really like how their syntax to construct an interpolator is interpolator(y, x) rather than (x, y) but that's a minor complaint. I have no experience with BSplineKit.jl but if it covers our needs (which seem relatively minor) I'm happy to use it.

Should this be wrapped into the 0.11.2 release? We updated the Project.toml in #50 but we can wait to register until after merging this if you want.

src/color_laws.jl Outdated Show resolved Hide resolved
src/color_laws.jl Outdated Show resolved Hide resolved
Comment on lines 265 to 269
# using type annotations so `interpolate` can be inferred
# TODO: Avoid using readdlm to avoid this issue altogether
data_x::Vector{Float64}, data_k::Vector{Float64}, data_delta_k::Vector{Float64}, data_sigma_k::Vector{Float64} = let data = readdlm(joinpath(datadep"F19", "F19_tabulated.dat"), skipstart=1)
(data[:, i] for i in 1:4)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the caveat that I'm not familiar with this code, it seems to me it is inefficient to re-read the data for every call to f19_invum.

Why don't we load it as a constant (something like const f19_data) at the module level? I guess that would force download the datadep when you load the package but that sounds fine to me. That would also allow it's type to be known.

Frankly the data file's not even that large so we could just copy-paste the raw data into the source file unless there's some licensing issue or something ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree. I was very tempted to just inline it while writing out that TODO. Will play around with some schemes later tn

Project.toml Show resolved Hide resolved
icweaver and others added 2 commits February 12, 2025 18:44
Co-authored-by: Chris Garling <[email protected]>
Co-authored-by: Chris Garling <[email protected]>
@icweaver
Copy link
Member Author

I've used DataInterpolations.jl when I had some data that their specialty interpolators were useful for. I don't really like how their syntax to construct an interpolator is interpolator(y, x) rather than (x, y) but that's a minor complaint. I have no experience with BSplineKit.jl but if it covers our needs (which seem relatively minor) I'm happy to use it.

Awesome, thanks for the real world feedback! I'll try it out tn

Should this be wrapped into the 0.11.2 release? We updated the Project.toml in #50 but we can wait to register until after merging this if you want.

Sure, thanks! This is shaping up to be a real nice release

@icweaver
Copy link
Member Author

Ok, so I went ahead pasted that F19 dataset directly in to keep this PR from being held up. It looks like Ian from the past put current me in this corner #32 (comment). I wonder if artifacts might be what's needed to track and work with datasets internally like this?

I also tried out DataInterpolations.jl and liked it a lot! The one and only test that it seems to fail on is this one with Measurements.jl:

# uncertainties
noise = rand(length(wave)) .* 0.01
wave_unc = wave noise
reddening = map(w -> @uncertain(law(w)), wave_unc)
@test Measurements.value.(reddening) ref_values[rv] rtol = 1e-3

F04: Test Failed at /home/mango/projects/DustExtinction.jl/test/color_laws.jl:343
  Expression: ≈(Measurements.value.(reddening), ref_values[rv], rtol = 0.001)
   Evaluated: [0.06347334097148892, 0.24927400409113415, 0.8672265293564794, 0.9853541153619346, 1.2275044405360622, 1.3918100306783647, 2.0826715828272953, 2.1872727768019695] ≈ [0.05967741935483871, 0.24903225806451612, 0.8670967741935485, 0.9854838709677419, 1.2274193548387098, 1.3919354838709679, 2.0825806451612903, 2.1874193548387093] (rtol=0.001)

Stacktrace:
 [1] macro expansion
   @ ~/.julia/juliaup/julia-1.10.8+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:672 [inlined]
 [2] macro expansion
   @ ~/projects/DustExtinction.jl/test/color_laws.jl:343 [inlined]
 [3] macro expansion
   @ ~/.julia/juliaup/julia-1.10.8+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [4] top-level scope
   @ ~/projects/DustExtinction.jl/test/color_laws.jl:319

I'm probably just holding it wrong. At any rate, I think this should be ready for review now

@icweaver icweaver marked this pull request as ready for review February 13, 2025 08:43
@giordano
Copy link
Member

I wonder if artifacts might be what's needed to track and work with datasets internally like this?

I haven't checked what's the format of the data you're dealing with, but a note about Artifacts is that it supports only tarballs, if you don't have a tarball to start with you'd need to repackage it and upload it somewhere, which isn't always a nice thing to do (but ArtifactsUtils.jl makes this stuff easier)

@cgarling
Copy link
Member

It's just a small matrix with a few kb of data (something like 4 columns, 100 rows). Since the data are so small I prefer this solution, and +1 for keeping the link to the raw data file in a comment so we know where it came from.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants