Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dynamic Programming c++ implementation #9
base: main
Are you sure you want to change the base?
Dynamic Programming c++ implementation #9
Changes from 8 commits
bb10165
d13dfc7
5bbbaf2
930430a
379897a
e35cff1
e8ccb7c
fccca35
0c64950
fe6bac1
245a92f
0af1b08
66e2104
a4cff6f
9ccf1fe
8da0fe0
943a6c0
98ee45c
744b928
b8dd0f2
c7e6920
7771157
f7287f8
47aedc0
4fd3a58
4dd27f8
f51ff1c
c0afe6f
77e3e00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Don't use the namespace, explicitly scope them. It makes the code more readable.
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.
If you use
std::
andEigen::
then you don't need these lines.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.
We could probably save some time by saving and computing the cumulative sum and then computing the mean through (
(sum_[end] - sum_[begin]) / (end - begin)
).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 were your thoughts @npkamath?
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.
We should compute the regression for all dimensions simultaneously; it should be faster. Also, is there a way to avoid the copying of data and have the prediction method take a reference to the data to store in?
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.
Had an attempt at it but it is not returning the right type to pass into the next function; I will try a different approach, maybe not with Eigen
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.
Make the constructor for
linear_fit_struct
set up the values.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.
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.
Shouldn't right always be one element long? If so, we could just push back.
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.
This should be known from the passed matrix no?
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.
Some of this was just for testing/setting back in ye old days of input.txt. I can just make it use data.size now
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.
Still to be done.
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.
If we keep this we should be more elaborate and use
num_features
andnum_breakpoints
.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.
Also, if possible, expand on the variable names throughout. It will help future developers work with the code.
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.
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 also think this would work better as a fleshed out class that had methods to fit and such my thoughts (though by no means the only way to do this) would be to
Create a
fit
method which computes the slopes and intercepts in aline struct
which has a call operator that returns y's for given x. The x's could be stored in the struct itself.If you have other ideas that is fine. I just like the idea of separating the logic for line fitting from the
DynP
class. Ideally, the entire cost function would be abstracted out for future development.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 like this idea, I'm thinking maybe I just create a regression.h file and just make a separate class? the dupin.h file is getting a little messy
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 think this would be a good idea, and hopefully not too much work as the code is pretty modular already.
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.
Most of these should be private or protected.
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.
Many of these methods either don't need to exist the
n_timesteps
n_parameters
stuff or should be private/protected. The constructor, setters, getters, and anything exported to Python should remain here.