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

New package: Fminbnd v1.0.0 #119764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JuliaRegistrator
Copy link
Contributor

@JuliaRegistrator JuliaRegistrator commented Nov 19, 2024

Copy link
Contributor

Hello, I am an automated registration bot. I help manage the registration process by checking your registration against a set of AutoMerge guidelines. If all these guidelines are met, this pull request will be merged automatically, completing your registration. It is strongly recommended to follow the guidelines, since otherwise the pull request needs to be manually reviewed and merged by a human.

1. New package registration

Please make sure that you have read the package naming guidelines.

2. AutoMerge Guidelines are all met! ✅

Your new package registration met all of the guidelines for auto-merging and is scheduled to be merged when the mandatory waiting period (3 days) has elapsed.

3. To pause or stop registration

If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment.

Tip: You can edit blocking comments to add [noblock] in order to unblock auto-merging.

@goerz goerz mentioned this pull request Nov 19, 2024
@goerz
Copy link
Member

goerz commented Nov 19, 2024

This seems to be the followup from #99531.

I'm not really clear on what the bnd part of the name should indicate.

@goerz
Copy link
Member

goerz commented Nov 19, 2024

I might tentatively suggest NetlibFmin as a name

@JuliaTagBot JuliaTagBot added the AutoMerge: last run blocked by comment PR blocked by one or more comments lacking the string [noblock]. label Nov 19, 2024
@goerz
Copy link
Member

goerz commented Nov 19, 2024

I'd also strongly recommend leaving the name of the function exported by the package as fmin.

@kagalenko-m-b
Copy link

kagalenko-m-b commented Nov 19, 2024

I might tentatively suggest NetlibFmin as a name

There’re widely used matlab/octave functions ‘fminbnd’ and python’s ‘fminbound’ that also derive from netlib’s fmin. “Bnd” means that it searches minimum on a bounded interval.

@goerz
Copy link
Member

goerz commented Nov 19, 2024

Hmm… okay, but you're very explicitly translating the Netlib code, which doesn't use that name 🤷

What I mean is: it very much seems like the name was chosen because it passes the automated name check, not because it's the most appropriate name for the package. But, I feel like a package name that indicates that this is the exact fmin function from Netlib would be a better route

@kagalenko-m-b
Copy link

it very much seems like the name was chosen because it passes the automated name check,

No. The name was chosen because matlab’s “fminbnd” is better known than netlib’s “fmin”.

@goerz
Copy link
Member

goerz commented Nov 19, 2024

Then you should rewrite the README to explain this. But I'm not sure it makes sense to say "Literal rewrite in Julia of Fortran routine fmin from Netlib." (which this is!) and then not have the function named fmin

@goerz
Copy link
Member

goerz commented Nov 19, 2024

I would also point out that your signature does not match the Matlab fminbnd function (wrong order of arguments), but it does match the signature of Netlib's fmin

UUID: b7bab496-830c-4b53-8aec-8b9a988c40b4
Repo: https://github.com/kagalenko-m-b/Fminbnd.jl.git
Tree: 73bb531146ac1fb06759b3374255894c6e6e3970

Registrator tree SHA: 17aec322677d9b81cdd6b9b9236b09a3f1374c6a
@jaakkor2
Copy link
Contributor

jaakkor2 commented Nov 20, 2024

[noblock] Same Brent's method seems to be available in Optim.jl, see https://julianlsolvers.github.io/Optim.jl/stable/user/minimization/#Minimizing-a-univariate-function-on-a-bounded-interval

@kagalenko-m-b
Copy link

kagalenko-m-b commented Nov 20, 2024

Same Brent's method seems to be available in Optim.jl, see https://julianlsolvers.github.io/Optim.jl/stable/user/minimization/#Minimizing-a-univariate-function-on-a-bounded-interval

Optim is a very large and complex package with many dependencies. For simple problems it may be desirable to have a standalone alternative.

@adienes
Copy link

adienes commented Nov 25, 2024

rule 8 of package naming guidelines:

  1. Avoid using a distinctive name that is already in use in a well known, unrelated project.

fminbnd is

  • distinctive
  • in use in MATLAB which is well known and
  • "unrelated" because you are not using its implementation

also rule 4

  1. Err on the side of clarity, even if clarity seems long-winded to you.

I think NetlibFmin seems much more clear to me what this package does given your testimony that this is a 1-1 rewrite of that function.

naming aside, and I'm sorry to be too critical, but I would be a bit hesitant to say that this meets the quality standards to register in General in the first place. This package consists of a single function that is completely illegible. Could you at least try to factor out some of the @gotos into traditional control flow? compare to for example scipy's implementation

@goerz
Copy link
Member

goerz commented Nov 25, 2024

I wouldn't come down quite as hard on this. I think if it implemented the same interface as Matlab and was documented as such (instead of a translation of the Netlib code), I'd be totally fine with Fminbnd.

Likewise, the way it is implemented now, NetlibFmin would be totally fine. I'm saying this coming from a Fortran background where Netlib is the go-to if you need some code that you don't want to write yourself (most of it being in the public domain). So I don't even object to a Julia package that translates an existing Netlib package, even if it borders on being trivially small in this case.

Could you at least try to factor out some of the @Gotos into traditional control flow?

With the direct reference to Netlib, it's kinda cute, and I don't think I'd mind. This is really as close of a translation of the original code as possible. It would be interesting to also write this with modern (haha) control flows and benchmark if there is any difference at all in performance.

@kagalenko-m-b
Copy link

“unrelated" because you are not using its implementation

Different implementation of the same algorithm is “unrelated”?

@kagalenko-m-b
Copy link

think if it implemented the same interface as Matlab and was documented as such (instead of a translation of the Netlib code), I'd be totally fine with Fminbnd

kagalenko-m-b/Fminbnd.jl@b52ae0c

@goerz
Copy link
Member

goerz commented Nov 26, 2024

Okay, that seems good to me… Just make sure that you retrigger the registration to really make sure that the correct commit gets tagged. Especially since you're using v1.0.0 for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge: last run blocked by comment PR blocked by one or more comments lacking the string [noblock]. new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants