search

SimonEnsemble / PorousMaterials.jl

Julia package towards classical molecular modeling of nanoporous materials
GNU General Public License v3.0
51 stars 11 forks source link

Change optim algo #117

Closed Surluson closed 4 years ago

Surluson commented 4 years ago

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 commented 4 years ago

Pull Request Test Coverage Report for Build 569

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Misc.jl 8 10 80.0%
<!-- Total: 8 10 80.0% -->
Totals Coverage Status
Change from base Build 433: -0.03%
Covered Lines: 1139
Relevant Lines: 1425
💛 - Coveralls
SimonEnsemble commented 4 years ago

looks good, and an imperative addition to make sure the optimization algo converges. :+1: 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 commented 4 years ago

Nice catch!

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