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

Fix errors relating to RecursiveArrayTools, Base.stack, and GaussianDistributions.jl #314

Merged
merged 21 commits into from
Jun 11, 2024

Conversation

nathanaelbosch
Copy link
Owner

@nathanaelbosch nathanaelbosch commented Jun 9, 2024

I believe SciMLBase changed something about how solution indexing and plotting works, and it rougly relies on

indexed_solution = sol(plott; idxs = batch_symbolic_vars) 
push!(tmp, indexed_solution[idxx, :])

This indexing with indexed_solution[idxx, :] failed on our arrays of Gaussians. After a lot of digging, I found that there were multiple issues:

  1. Base didn't quite know how to handle the Gaussian type.
  2. GaussianDistributions.jl implemented an iterator interface, to allow people to do
    mean, cov = Gaussian(1, 0.1)
    Base.stack did not interact well with this.

I have fixed this by removing GaussianDistributions.jl and instead implementing our own minimal Gaussian type which brings everything we need (and not much more), which could possibly me moved out into a separate SimpleGaussians.jl package in the future. According to #278 this might also fix some more type piracies (not checked yet though).

Left to do:

  • clean up the code in src/gaussians.jl 🧹
  • run tests 🧪
  • review PR 📖
  • merge 🚀

Copy link

codecov bot commented Jun 9, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 28 lines in your changes missing coverage. Please review.

Project coverage is 91.84%. Comparing base (0525b2d) to head (eb40e03).
Report is 2 commits behind head on main.

Files Patch % Lines
src/gaussians.jl 61.01% 23 Missing ⚠️
src/solution.jl 80.00% 3 Missing ⚠️
src/filtering/update.jl 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
- Coverage   92.92%   91.84%   -1.08%     
==========================================
  Files          44       44              
  Lines        2147     2219      +72     
==========================================
+ Hits         1995     2038      +43     
- Misses        152      181      +29     

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

src/solution.jl Outdated Show resolved Hide resolved
src/diffusions/calibration.jl Show resolved Hide resolved
test/solution.jl Outdated Show resolved Hide resolved
src/derivative_utils.jl Show resolved Hide resolved
@nathanaelbosch nathanaelbosch merged commit a991f99 into main Jun 11, 2024
5 of 7 checks passed
@nathanaelbosch nathanaelbosch deleted the fixtests branch June 11, 2024 07:04
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.

1 participant