Skip to content

Comments

Converts "fitParams" and "otherParams" to row arrays#301

Merged
DrPaulSharp merged 1 commit intoRascalSoftware:masterfrom
DrPaulSharp:transpose
Jan 2, 2025
Merged

Converts "fitParams" and "otherParams" to row arrays#301
DrPaulSharp merged 1 commit intoRascalSoftware:masterfrom
DrPaulSharp:transpose

Conversation

@DrPaulSharp
Copy link
Collaborator

This PR addresses the issue in #299

The previous PR #257 unified the code to use column arrays for problemStruct.fitParams following the lead of the packParams routine. However, this bug has arisen because the de and dream algorithms expect fitParams to take row arrays. However, simplex expects fitParams to take column arrays.

The best and least intrusive solution to this is to refactor the code, including packParams, to use row arrays, with two changes to simplex to enable it to work correctly.

Copy link
Collaborator Author

@DrPaulSharp DrPaulSharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on what we've found with this bug, we also need to consider how we test the translation of python to C++ and back, and we need a basic test of "simplex", "de", "ns" and "dream" calculations in the python too - it seems like this bug exposed a seg fault there that has gone unnoticed until now.

Copy link
Collaborator

@StephenNneji StephenNneji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the description that changing the simplex to used row arrays should be the easiest route to fix this problem but this PR changes everything (there is also changes to the DE and dream) . Did something change the described direction?

@DrPaulSharp
Copy link
Collaborator Author

Yeah, I did have to switch tack on this one to keep things as simple as possible. I'll just go through and verify everything and then update as necessary.

@DrPaulSharp
Copy link
Collaborator Author

Ok, this PR changes everything to row arrays in the code outside of the minimisers. There are changes to simplex to ensure that the column arrays it uses are transposed to row arrays outside of the minimiser. There are also changes to the other minimisers to make sure that they convert (or not) to row arrays outside of the minimiser, rather than working with columns outside the minimiser as was previously done. Does that cover everything you wanted to know?

Copy link
Collaborator

@StephenNneji StephenNneji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, sorry for the confusion, I'm probably just recovering from my break 😄

@DrPaulSharp DrPaulSharp merged commit 6a9588d into RascalSoftware:master Jan 2, 2025
@DrPaulSharp DrPaulSharp deleted the transpose branch January 2, 2025 13:03
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.

2 participants