Skip to content

New algorithm #16

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

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

New algorithm #16

wants to merge 3 commits into from

Conversation

putianyi889
Copy link

@putianyi889
Copy link
Author

putianyi889 commented Mar 27, 2022

The codes were directly translated from the original matlab codes. I haven't studied the algorithm or done any Julia localization.

@jlapeyre
Copy link
Collaborator

jlapeyre commented Aug 5, 2022

Thanks for the contribution! I will review it.

@jlapeyre
Copy link
Collaborator

jlapeyre commented Aug 5, 2022

This looks good. Even thought it's translated, it does not look like unidiomatic Julia for the most part.

Does it work? It could use some kind of test.

It looks like mittleff_matlab is kind of a placeholder name. I can't think of anything better off the top of my head.

Also, if this is better than the existing code in all regimes, then of course it could replace the old implementation/algorithm.

@putianyi889
Copy link
Author

putianyi889 commented Aug 5, 2022

The only problem I can see is that the default tolerance is set to $10\varepsilon$, which means that I had to use BigFloat in practice to get the last bits right. Otherwise the results are correct and the performance is consistent.

I didn't add any test since I was not sure how you would manage the codes.

I'm not exactly sure when mittleff_matlab is better as I've not studied either of the algorithms. The old implementation, either better or worse, could remain there for reference, e.g., test that the two algorithms return the same results.

@jlapeyre
Copy link
Collaborator

I find that this routine is ten times slower than the current one for some parameters

julia> @btime mittleff_matlab(0.3, 1.0, .4)
  11.311 μs (96 allocations: 10.83 KiB)
1.725742301414842

julia> @btime mittleff(0.3, 1.0, .4)
  1.408 μs (0 allocations: 0 bytes)
1.7257423014148419

and similar for other parameters. But I don't doubt that behavior in various regions of the three-parameters space is complicated. I think it's ok to commit this with a few changes

  • Some kind of testing. Doesn't have to be great. The current code has poor testing. But I think I compared with Mma output for a couple of touchstones.
  • A more descriptive name, even mittleff_alt or something. matlab doesn't give useful information.
  • A docstring that gives a ref to the paper and says something about it being an alternative implementation
  • Something in the README noting that these two implementations exist.

There are a few things that could be tried to improve efficiency of the "matlab" version. But the current state of the code is rather crude and not optimized. So I would not hold this up based on efficiency.

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 155 lines in your changes missing coverage. Please review.

Project coverage is 36.82%. Comparing base (1059181) to head (76f1587).

Files with missing lines Patch % Lines
src/matlab.jl 0.00% 155 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1059181) and HEAD (76f1587). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (1059181) HEAD (76f1587)
10 9
Additional details and impacted files
@@             Coverage Diff             @@
##           master      #16       +/-   ##
===========================================
- Coverage   77.30%   36.82%   -40.49%     
===========================================
  Files           2        3        +1     
  Lines         141      296      +155     
===========================================
  Hits          109      109               
- Misses         32      187      +155     

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

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.

2 participants