search

ethz-asl / curves

A library of curves for estimation.
BSD 3-Clause "New" or "Revised" License
74 stars 28 forks source link

Feature/gp curves #35

Closed sanderson77 closed 9 years ago

sanderson77 commented 9 years ago

The majority of this branch is the addition of several GP related files.

It also contains: 1) the addition of an optional output parameter in fitcurve 2) slight modification to the CMake setup... for CMake based IDE's (I am using Qtcreator), header files are not visible unless specified.

I don't believe any of these changes will break the master branch of the continuous-scan matching code.. but my feature/gp-optimization builds properly.

furgalep commented 9 years ago

Hi Sean, I'll review this by the end of the weekend. OK?

sanderson77 commented 9 years ago

That is fine.

I'm fairly new to git, what is the preferred workflow if I wanted to make additional changes before you have a chance to review? keep pushing to the same branch?

furgalep commented 9 years ago

Good question. Here is what will happen:

I (and maybe others) will go through your code and comment on lines/files/structure/comments etc. You should address these comments and push these back to this branch.

If you develop further in the meantime and it isn't part of this review, do it in a new branch that is split off from this one. Then you can always merge changes from this branch back to that one.

When this branch finally gets merged to master, then you can make a PR for your new code. Git handles this all very well. One note: it is nice to your reviewers to keep PRs as small and as focused as possible. So, just implement one thing, make a PR, implement the next thing, make a PR. PRs that are this big are a lot to take in all at once.

OK!

sanderson77 commented 9 years ago

Sorry about that, I'll try to keep them smaller in the future.

furgalep commented 9 years ago

So, aside from the possible memory leaks, this looks OK.