Style changes #344
Style changes #344
Conversation
There was a problem hiding this comment.
I did a really quick run through, I initially marked the methods as not-changed-but-should-be but then later noticed you didn't do those. I removed those comments, but just a heads up if there's one or two I missed to ignore them.
Overall the most confusing bit to me were the local variables. Sometimes camelCased sometimes snake_cased - it wasn't always clear what the strategy was. Generally, I'll admit that I'm usually a camelCaser, but PEP8 says underscores for both local vars and funcs and camel-case only for classes and Google C++ style you linked also underscores all local variables, unless they're type names in which case capitalized camel case, and in case they're class members we end them with an extra trailing _. So we should probably just snake_case everything except classes too.
No need to rush with this because it's something we can do alongside the dev-work. I think the ones that hurt a lot are the ones that are exposed to the user in the interface, but renaming those includes changing Py code.
In general, clarity of naming (f.e. PointSpreadFunc.printPSF and hungarian-but-not float pyMinLH) should be improved upon more so than the standard. In any case, an improvement even as-is, so approved.
| LayeredImage::LayeredImage(std::string path, const PointSpreadFunc& psf) : psf(psf), psfSQ(psf) { | ||
| psfSQ.squarePSF(); | ||
| LayeredImage::LayeredImage(std::string path, const PointSpreadFunc& psf) : psf(psf), psf_sq(psf) { | ||
| psf_sq.squarePSF(); |
There was a problem hiding this comment.
missed squarePSF? It's in thePointSpreadFunc - which, if we're renaming stuff we should probably just call PSF tbh.
| extern "C" void deviceSearchFilter(int num_images, int width, int height, float* psi_vect, float* phi_vect, | ||
| PerImageData img_data, SearchParameters params, int num_trajectories, | ||
| trajectory* trj_to_search, int num_results, trajectory* best_results); | ||
|
|
There was a problem hiding this comment.
Unrelated to the current PR, just a general comment. These big blocks of params make it very difficult for me to see the changes. I know it's not C++ style but if we could do a one-per-line after 5 params that would make it really easy to see what changes in a PR and what the func expects. As well as allow inline annotations if/when we need them.
| deviceConvolve(pixels.data(), pixels.data(), getWidth(), getHeight(), psf.kernelData(), psf.getSize(), | ||
| psf.getDim(), psf.getRadius(), psf.getSum()); |
There was a problem hiding this comment.
just re: earlier proposal of one-per-line-ing this we could have a inline comment or just direct named parameters that would then distinguish between pixels.data() and pixels.data() in this context.
This is the first step of making the coding style more consistent.
This PR is a mostly mechanical style changes to the C++ files to: