-
Notifications
You must be signed in to change notification settings - Fork 2
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
Nlog bmdu #67
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! a few minor edits and recommendations
src/code_base/bmds_helper.cpp
Outdated
@@ -9,6 +9,7 @@ | |||
#include <ctime> | |||
//#include <cmath> | |||
#include <nlopt.hpp> | |||
#include <functional> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed. It was for something I had temporarily put in and then removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it be removed? it looks like it's still included
src/code_base/bmds_helper.cpp
Outdated
@@ -4520,6 +4519,51 @@ void Nlogist_BMD(struct python_nested_analysis *pyAnal, struct python_nested_res | |||
} | |||
pyRes->bmdsRes.BMDL = BMDL; | |||
|
|||
|
|||
//BMDU test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general comment - could this be written as an isolated function calcNlogisticBmdu(objData)
instead of inline a really large function? it may make it easier to refactor in the future. This may not make sense in C++, apologies in advance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this. This was definitely needed, but I've been putting this off just to get the code running. There will be many other places that need it also. I'll circle back and do more refactoring if I have the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Added bmdu calc for nlogistic model