Refactor bindings. #346
Merged
Refactor bindings. #346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DinoBektesevic
force-pushed
the
refactor_bindings
branch
2 times, most recently
from
5c46d9b to
e9e5876
Compare
DinoBektesevic
force-pushed
the
refactor_bindings
branch
2 times, most recently
from
17c096d to
470ace2
Compare
I forgot to split up this commit into i) prefactoring method names ii) splitting bindings iii) fixing docs iv) fixing Python references but it's all been done.
KBMOSearch has been renamed StackSearch to match Python bindings. I have no preference regarding the naming of the class, it can rename KBMOSearch - as long as it's consistent with its Python bindings. In general no other class had method names as confusing or as disparate from its Python bindings so in this merge, I've renamed multiple of its methods - which was not the case for other classes in this refactor. The following renames occured: * get_image_stack -> get_imagestack * scienceStampsForViz, science_viz_stamps -> get_stamps * [science, sci, mean, median, coadd, summed]_[sci, science]_stamp[s] -> [mean, median, coadd, summed]_stamp[s] * get_traj_pos, getTrajPos -> get_trajectory_position * get_mult_traj_pos, getMultTrajPos -> get_trajectory_positions * [psi, phi]_curves -> get_[psi, phi]_curves Same like with the class name, I'm not married for either particualar individual variation on the topic, but the **only** kind of indirection that should occur in the bindings is to make the interface feel and act more Pythonic, by for example wrapping [get, set]_method using a pybind11::def_property and then not exposing the individual getters and setters, or for example, wrapping print, stringify and similar methods in the dunder str, dunder repr methods and then also not exposing the internals in the binding code.
… ImageStack and StackSearch.
Rename the struct to make it compliant with style guide. Fix the missmatched binding names. Fix all the tests.
…ny clarity or brevity.
It seems there were more than a few items I missed renaming during the rebase and I fat-fingered a couple of , instead of a . Fixes bindings for add_pixel_interp in RawImage.
DinoBektesevic
force-pushed
the
refactor_bindings
branch
from
470ace2 to
4e59b2e
Compare
jeremykubica
approved these changes
Sep 25, 2023
| i++; | ||
| } | ||
|
|
||
| radius = i - 1; // This value is good for |
There was a problem hiding this comment.
Is there more to this comment? Or was it a question?
There was a problem hiding this comment.
Great question, it first appeared in 506d347 6 years ago and has been carried along ever since...
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
With all the recent refactoring I thought I'd join in on the fun.
Here is an initial pass through the code, making it (hopefully) more compliant to the google's C++ style guide. The refactoring:
bindings.cppby separating the bindings into factory methods located next to the implementationKBMOSearchintoStackSearchto match the Python bindings (see full 7b2d479 commit message for reasoning, this isn't set in stone)getDataRefvariants, replacing them with.data()where necessaryx_v)I just noticed that GH shows all the renamed files as new files. Moving files with git, registered them as renames. Rest assured that per-line commits and provenance is still there, you can verify for yourself. This seems to be more a quirk of GitHub not being able to correctly display it. I think this occurred because I moved them and then edited them before committing the move as its own commit. If people feel very passionate about it, I can go back and rebase and split up the
git mv's into separate commits, but, again, like I said if you clickview fileand check outgit blameeven GH will display the full history. Git locally doesn't seem to suffer from this.Main does not build
I think our build on main is broken. At least it doesn't work on baldur. Both
nvccandnvidia-smiare visible in the environment and addition of print statement and runningpipin verbose mode shows that it's building "with the GPU". However, there are tests that are missingskipIfprotection, a print statement that wasn't scoped topybind, but also for some reason theconstexprforHAS_GPUdoesn't evaluate correctly. The IPO checks throw a warning too.This PR "fixes" the broken build in so much that it fixes the bare
printstatement and adds protection to whereversearchis called in tests so they don't error out. What it doesn't fix is theHAS_GPUflag, which evaluates toFalse. I haven't investigated the reasons in order to finish the refactor, but this should be a priority. Steps to reproduce on baldur:Even when fixed and confirmed the build executes with the GPU found by running
pip install -v .and adding the print statements tosetup.py