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

Removed curve_fit #56

Closed
wants to merge 1 commit into from
Closed

Conversation

cmcbride
Copy link

@cmcbride cmcbride commented Jun 1, 2014

This is to support the discussion of #55 and remove the functionality that was broken out into CurveFit.

There were two commits: one that removed curve_fit (and the Distributions dependency) and a followup that removed levenberg_marquardt.

This is just a small simplification, as the curve_fit functionality
was broken out into CurveFit.jl.
@rsrock
Copy link
Contributor

rsrock commented Jun 1, 2014

Guess I should have looked here before filing #57...

@cmcbride
Copy link
Author

cmcbride commented Jun 2, 2014

OK, backed out one commit to the respective branch and this PR to keep levenberg_marquardt() within Optim, as discussed in #55.

@cmcbride
Copy link
Author

cmcbride commented Jun 8, 2014

As soon as JuliaOpt/CurveFit.jl is created, this PR should be applied. Do you agree @mlubin ?

@johnmyleswhite
Copy link
Contributor

Once CurveFit.jl is released, I'm happy to merge this.

@mlubin
Copy link
Contributor

mlubin commented Jun 9, 2014

@cmcbride, LGTM. I've given you temporary admin access to move CurveFit.jl into JuliaOpt.

@cmcbride
Copy link
Author

cmcbride commented Jun 9, 2014

@mlubin Didn't realize that was necessary, but I can do that no problem. Should I "transfer ownership", fork it into JuliaOpt, or just git push it? I'm sure there are subtle github differences to each...

@mlubin
Copy link
Contributor

mlubin commented Jun 9, 2014

@cmcbride I think the best way is to move over the original repo itself.

@cmcbride
Copy link
Author

cmcbride commented Jun 9, 2014

OK, @mlubin, it's transferred. JuliaOpt/CurveFit.jl now exists.

Maybe CurveFit.jl should be listed in METADATA before this PR removes the curve_fit from Optim.jl? That sound reasonable @johnmyleswhite?

@johnmyleswhite
Copy link
Contributor

Yeah, that's reasonable. Just let us know when that happens.

@cmcbride
Copy link
Author

cmcbride commented Jun 9, 2014

Uh oh, guess we failed doing our homework: CurveFit.jl exists in METADATA as:
https://github.com/pjabardo/CurveFit.jl

Thoughts for a new name?

Also, this conversation is starting to feel OT for this issue.

@mlubin
Copy link
Contributor

mlubin commented Jun 9, 2014

How do the packages differ in functionality? JuliaOpt/CurveFit seems more general

@cmcbride
Copy link
Author

@mlubin I agree JuliaOpt/CurveFit seems more general. But they appear to be different takes on the same niche, so it makes sense to just rename this package (which was conceived a bit after the other ones creation). As the author of (the first) CurveFit.jl, what do you think @pjabardo?

Possibilities:

  1. SimpleFit
  2. ModelFit
  3. Fitter (I'm not really a fan of this one)
  4. (suggestions?)

@mlubin
Copy link
Contributor

mlubin commented Jun 10, 2014

ModelFit sounds fine to me, but I'd be much more in favor of combining the two packages. We'll see what @pjabardo says.

@IainNZ
Copy link
Contributor

IainNZ commented Jun 11, 2014

I feel like in general that the "better developed" package should get the name, its looks like from http://pkg.julialang.org/?pkg=CurveFit&ver=0.3 that it doesn't even work. But a merge would be best.

@mlubin
Copy link
Contributor

mlubin commented Jun 11, 2014

Hopefully we can resolve this by discussion with @pjabardo before resorting to any drastic measures. I've opened an issue on pjabardo/CurveFit.jl.

@cmcbride
Copy link
Author

it does not look like @pjabardo is responding quickly. Perhaps we can consider a name change to ModelFit and work on merging pjabardo/CurveFit.jl at a later date?

@mlubin
Copy link
Contributor

mlubin commented Jun 16, 2014

Given that merging/renaming packages in METADATA is a bit of a mess right now, I think its worth trying to resolve this first, but if others would prefer to just rename it for now that's okay with me. We don't really have a policy for unresponsive package authors, though it's something that we'll have to deal in general with sooner or later.

@IainNZ
Copy link
Contributor

IainNZ commented Jun 16, 2014

I was thinking of bringing it up at JuliaCon

@pjabardo
Copy link

Sorry for the delay. I was out hiking and without Internet connection for the past two weeks. I fixed the problem on the repository three weeks ago but I experienced some trouble updating METADATA.jl, being new to git and all. I am not home but I will try to install julia (and git) on a computer today or tomorrow.

One issue that I've been having is with managing the packages. I find the idea of using git/GitHub brilliant but for package writers the docs are not clear and from my experience, Pkg.publish only works if one has write access to METEDATA. It is kind of confusing. A short recipe on how to update a package on metadata.jl would be helpful.

Sorry for the mess and for disappearing.

Paulo


Em dom, 15 de jun de 2014 03:22 BRT Cameron McBride escreveu:

it does not look like @pjabardo is responding quickly. Perhaps we can consider a name change to ModelFit and work on merging pjabardo/CurveFit.jl at a later date?


Reply to this email directly or view it on GitHub:
#56 (comment)

@cmcbride
Copy link
Author

Hi Paulo (@pjabardo) -- are you interested in merging your CurveFit.jl with the JuliaOpt/CurveFit.jl?

@mlubin -- my intention was to keep the name ModelFit and just merge the pjabardo/CurveFit.jl functionality in as necessary to (hopefully) remove duplicity and join efforts. This way there is no pressure and room for stylistic differences. (I'm trying to be very neutral here.)

@StefanKarpinski
Copy link

Given that merging/renaming packages in METADATA is a bit of a mess right now, I think its worth trying to resolve this first, but if others would prefer to just rename it for now that's okay with me.

Can you elaborate on what the problem with this is? Renaming a package seems to work fine as long as you don't immediately make a new, unrelated package with an old name like happened with the whole StatsBase / Stats fiasco. Now that was a mess, I have to agree.

@mlubin
Copy link
Contributor

mlubin commented Jun 16, 2014

@StefanKarpinski How clean would it be if we published ModelFit and then eventually it was renamed and replaced CurveFit after integrating the functionality of pjabardo/CurveFit.jl?

@StefanKarpinski
Copy link

It wouldn't be a problem – again, as long as you don't create a new, unrelated package with the same name, at least without letting some time elapse between (say one release cycle).

@cmcbride
Copy link
Author

The more I think about it, the more ModelFit seems like a better overall name.

@lindahua
Copy link
Contributor

ModelFit reminds me of fitting a distribution to observed samples (e.g. MLE), which is a completely different can of worms.

@cmcbride
Copy link
Author

@lindahua fair enough. It's definitely not a likelihood estimator -- just a minimizer. Funny how "Curve" works well just because it's such a superficial word, so people know what to expect.

@cmcbride
Copy link
Author

FWIW, I strongly agree with @mlubin. I believe the time to try to keep things consolidated is now, as the number of julia packages will only explode over time making the situation worse.

My opinion: the merger is worth pushing for. I think the question is when and how. I skimmed the codes, and it seems easy to implement a solution that will make both project directions happy. Maybe it would be helpful to consider two possibilities:

  1. Merge now to keep the CurveFit name?
    As @pjabardo package is in METADATA, it seems the least problematic option is to use that as a base to submit pull-requests to. (Users will then just get FF updates).
  2. Start JuliaOpt/XXXFit (or whatever name that is different than CurveFit) and merge slowly
    The idea here would be to incorporate pjabardo/CurveFit functionality and eventually remove that package.

(I think these address the issues @StefanKarpinski mentioned above.)

If (1) is the chosen solution, I think it makes sense to abandon the current JuliaOpt/CurveFit.jl repo which can cause confusion to people that do a Pkg.update(). Besides the name, I don't see a reason to rush a merge, thereby making (2) the reasonable choice.

CurveFit is nicely generic, but I am sure we can come up with some decent alternative names. Taking into account the association that @lindahua pointed out (does ModelFit imply MLE?), perhaps SimpleFit or MinFit (which could stand for "minimizer fitting" or "minimal fitter") might work.

I'm happy with either possible solution. Someone just has to decide. @pjabardo? @mlubin? someone else?

@johnmyleswhite
Copy link
Contributor

Hate to do this, but is ModelFit or CurveFit always going to just be non-linear least squares? Calling that ModelFit strikes me as way too generic. This is part of the reason I recommended looking at NLreg.jl: its name matches what it really offers.

@lindahua
Copy link
Contributor

What about LeastSquareFit or LsqFit, which seems to accurately reflect what the package does.

@johnmyleswhite
Copy link
Contributor

That seems like a much better name to me.

@pjabardo
Copy link

Yes. I think this is probably the best solution.
LsqFit is a good name.

Paulo

Em Quarta-feira, 18 de Junho de 2014 20:26, Dahua Lin notifications@github.com escreveu:

What about LeastSquareFit or LsqFit, which seems to accurately reflect what the package does.

Reply to this email directly or view it on GitHub.

@cmcbride
Copy link
Author

I like either of those.

@blakejohnson
Copy link
Contributor

LsqFit and call it a day?

@mlubin
Copy link
Contributor

mlubin commented Jun 18, 2014

LsqFit sounds good to me. Should I rename JuliaOpt/CurveFit.jl to JuliaOpt/LsqFit.jl, or would you rather start from scratch?

@cmcbride
Copy link
Author

I think a rename should be fine. I'll push a change with the internal name changes.

@mlubin
Copy link
Contributor

mlubin commented Jun 18, 2014

Done, both you and @pjabardo have commit access.

@cmcbride
Copy link
Author

OK, as soon as we get LsqFit.jl listed in METADATA then I think this PR is ready to apply.

@IainNZ
Copy link
Contributor

IainNZ commented Jul 3, 2014

What was the final decision on this? I got a bit lost

@johnmyleswhite
Copy link
Contributor

I'm not sure myself.

@cmcbride
Copy link
Author

cmcbride commented Jul 5, 2014

Sorry, I've been consumed with work and a family move over the past couple weeks.

This patch (or equivalently removing the functionality) should be applied, and was waiting for JuliaOpt/LsqFit to be listed in METADATA (which I never got to submitting). Someone else is welcome to submit LsqFit.jl to METADATA, and then apply this. Otherwise, I'll try and look at it sometime in a week-ish.

@lindahua
Copy link
Contributor

lindahua commented Jul 6, 2014

@cmcbride would you please go ahead to publish LsqFit? It seems to me that there's no objection to this.

@blakejohnson
Copy link
Contributor

If someone wants to grant me access to LsqFit, I'd be happy to publish it.

On Sunday, July 6, 2014 at 7:59 AM, Dahua Lin wrote:

@cmcbride (https://github.com/cmcbride) would you please go ahead to publish LsqFit? It seems to me that there's no objection to this.


Reply to this email directly or view it on GitHub (#56 (comment)).

@mlubin
Copy link
Contributor

mlubin commented Jul 7, 2014

Done @blakejohnson

@cmcbride
Copy link
Author

cmcbride commented Jul 7, 2014

That'd be great, @blakejohnson. Like I said, I'm still fairly consumed by a number of work/home issues (waaaay behind at work at the moment).

@johnmyleswhite
Copy link
Contributor

Bump. I think LsqFit is available now.

@blakejohnson
Copy link
Contributor

Yes, I think this can be merged.

@johnmyleswhite
Copy link
Contributor

It needs some rebuilding to be put into a merge-able state.

@mlubin
Copy link
Contributor

mlubin commented Jul 25, 2014

Bump again. @cmcbride, could you resolve the merge conflicts?

@cmcbride
Copy link
Author

OK, I'll rebase and resolve the merge issues. Sorry -- I've been pretty busy at work, but I'll have a look at it over the weekend.

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

9 participants