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.

Sign up for GitHub

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

Jump to bottom

NonlinearObjectiveFunction.SetObserved method throws overflow exception for large weighted vectors #926

Open
AutWint opened this issue May 24, 2022 · 4 comments · May be fixed by #927
Open

NonlinearObjectiveFunction.SetObserved method throws overflow exception for large weighted vectors #926

AutWint opened this issue May 24, 2022 · 4 comments · May be fixed by #927

Comments

@AutWint
Copy link

AutWint commented May 24, 2022 •
edited
Loading

I'm using the NonlinearObjectiveModel for nonlinear least squares fitting of weighted data. However, the fitting fails due to an overflow exception when the number of data points becomes too large (let's say, 10000+).

The origin of this exception is the SetObserved method in the NonlinearObjectiveFunction. At the end of this method, a DenseMatrix is constructed for the data point weights, although this matrix contains diagonal elements only. The dense matrix stores all (10000+)^2 (mostly zero) entries, which exceeds my memory. To my mind, a DiagonalMatrix would be the better choice here.

I've tested replacing line 217-219 with

Weights = (weights == null)
    ? null
    : Matrix<double>.Build.DiagonalOfDiagonalVector(weights);

... and it solves my issue. I also tested this for small data (10 points) and didn't notice any performance declines there either.

Is there any reason for hanging on to the DenseMatrix? If not, I would suggest modifying the mentioned lines as described. I'm happy to great a pull request for this.
@diluculo
Copy link
Contributor

diluculo commented May 24, 2022 •
edited
Loading

@laidBackBird, you are right.

The weights matrix, W is only used to decompose into W = LL', i.e. L = Weights.Diagonal().PointwiseSqrt()

@AutWint
Copy link
Author

AutWint commented May 24, 2022

@diluculo That's true, I didn't follow it further. Storing L should be sufficient, since the Weights apparently aren't used otherwise. I could imagine that keeping them was a "consistency choice"?!

How should we proceed? Should I implement the (minor) changes and create a pull request? If so, would you suggest removing the Weights property from the class?

@diluculo
Copy link
Contributor

@laidBackBird, the 'Weights' is not only set initially but can be changed at each iteration step in some cases. That is, it needs to be verified later. It is why I made 'Weights' public property.

@AutWint AutWint linked a pull request May 30, 2022 that will close this issue
Open
@AutWint
Copy link
Author

AutWint commented May 30, 2022

@diluculo I see. I've created a pull request following my initial "minimally invasive" idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature/fix overflow exception when fitting large data AutWint/mathnet-numerics
2 participants
@diluculo @AutWint