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

Change optim algo #117

Merged
merged 5 commits into from
Sep 17, 2019
Merged

Change optim algo #117

merged 5 commits into from
Sep 17, 2019

Conversation

Surluson
Copy link
Collaborator

I fixed the deprecated DataFrame calls in Misc.jl and changed the optimization algorithm to NelderMead for Langmuir fit. The LBFGS was unable to provide results for some isotherms.

I also added a check to see if the optimization algorithm succeeded or not.

Another option is using regular Least Squares (which is not included in Optim.jl for some reason). Thoughts?

@coveralls
Copy link

coveralls commented Sep 16, 2019

Pull Request Test Coverage Report for Build 569

  • 8 of 10 (80.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 79.93%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Misc.jl 8 10 80.0%
Totals Coverage Status
Change from base Build 433: -0.03%
Covered Lines: 1139
Relevant Lines: 1425

💛 - Coveralls

@SimonEnsemble
Copy link
Owner

looks good, and an imperative addition to make sure the optimization algo converges. 👍
the objective function is least squares, just non-linear, so Nelder-Mead is a good algo to use.

@huynmela and @Surluson can you please add one of the isotherm data sets that failed the first time as a test for the Langmuir fitting? I know you don't know what should be the "right" answer but just chose a data set where it failed fitting the first time, but there is a good amount of curvature in the data to be reasonably confident that the fit is correct.

@Surluson I think I found a bug in the Langmuir that could explain the failure to converge. Could you try fixing this and see if it works? Fine to keep the Nelder Mead algo if it works better with poorer initial guesses. But I suspect this poor initial guess is what prevented convergence.
K0 = M0 * H0
So N = MKP/(1+KP) is Langmuir model. at low P, N ~ MKP so the henry coefficient is MK. That means a good guess for K is the henry coefficient divided by M, right? If I am right, please fix this line in addition to pushing the new data for testing. curious to know if it solves the problem!

@Surluson
Copy link
Collaborator Author

Nice catch!

I added another check in the misc_test.jl file and fixed the bug 👍
Surprisingly, fixing the bug didn't seem to fix the issue we had before (using LBFGS as the optimization algorithm), so NelderMead it is!

@SimonEnsemble SimonEnsemble merged commit 7774bce into master Sep 17, 2019
@SimonEnsemble SimonEnsemble deleted the change_optim_algo branch September 17, 2019 03:07
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.

4 participants