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

Remove direct estimagic dependencies to avoid circular imports #6

Merged
merged 3 commits into from
May 22, 2023

Conversation

janosg
Copy link
Member

@janosg janosg commented May 20, 2023

Remove direct estimagic dependencies to avoid circular imports

  • Use scipy minimize in optimal sampling
  • Write custom process batch evaluator
  • Hardcode some algo options
  • Look at benchmarks

The benchmarks change slightly. This is expected, as the optimization in the optimal sampler changed:

  • scipy numerical derivative instead of estimagics's
  • slightly different convergence criteria

I think we can ignore this. The changes are smaller than differences we get between computers.

Old parallel ls

image

New parallel ls

image

Old noisy ls

image

New noisy ls

image

Old ls

image

New ls

image

Old scalar

image

New scalar

image

@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #6 (43735cd) into main (4bce6be) will decrease coverage by 0.06%.
The diff coverage is 80.85%.

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
- Coverage   77.96%   77.91%   -0.06%     
==========================================
  Files          69       68       -1     
  Lines        4852     4881      +29     
==========================================
+ Hits         3783     3803      +20     
- Misses       1069     1078       +9     
Impacted Files Coverage Δ
src/tranquilo/tranquilo.py 97.56% <ø> (-0.06%) ⬇️
src/tranquilo/wrap_criterion.py 81.25% <58.33%> (-13.99%) ⬇️
src/tranquilo/process_arguments.py 94.30% <60.00%> (-3.06%) ⬇️
src/tranquilo/options.py 97.88% <100.00%> (ø)
src/tranquilo/sample_points.py 98.34% <100.00%> (+0.08%) ⬆️
tests/test_fit_models.py 100.00% <100.00%> (ø)
tests/test_tranquilo.py 100.00% <100.00%> (ø)
tests/test_visualize.py 100.00% <100.00%> (ø)

@janosg janosg requested a review from timmens May 20, 2023 14:59
Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

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

Looks good to me. I only have one question about an estimagic import, but I've already approved it.

src/tranquilo/wrap_criterion.py Show resolved Hide resolved
@janosg janosg merged commit 7042550 into main May 22, 2023
12 checks passed
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.

None yet

2 participants