MET-34 Blender #18
MET-34 Blender #18
Conversation
MET-34 Implement `Blender`
Definition of done
|
genmetaballs/src/cuda/bindings.cu
Outdated
| .def_rw("beta4", &TwoParameterConfidence::beta4) | ||
| .def_rw("beta5", &TwoParameterConfidence::beta5) |
There was a problem hiding this comment.
would be good to use def_ro for immutable types.
genmetaballs/src/cuda/bindings.cu
Outdated
| .def(nb::init<float, float, float>()) | ||
| .def_rw("beta1", &ThreeParameterBlender::beta1) | ||
| .def_rw("beta2", &ThreeParameterBlender::beta2) | ||
| .def_rw("eta", &ThreeParameterBlender::eta) |
| float eta; | ||
|
|
||
| CUDA_CALLABLE __forceinline__ float blend(float t, float d) const { | ||
| return expf((beta1 * d * sigmoid((beta3 / eta) * t)) - ((beta2 / eta) * t)); |
| "TwoParameterConfidence", | ||
| "sigmoid", | ||
| "FourParameterBlender", | ||
| "ThreeParameterBlender", |
There was a problem hiding this comment.
Instead of directly exposing the confidence methods, let's have a .blender
horizon-blue
left a comment
horizon-blue
left a comment
There was a problem hiding this comment.
This mostly LGTM except some minor nitpicking :). Feel free to merge then you're ready.
One thing I started to realize for AI-generated tests is that while the are very comprehensive, they can be quite distracting as well. As a reviewer, I often ended up spending the majority of my reviewing cycles trying to decipher what we are trying to do.
Aside from testing for correctness, unit tests are often very useful in conveying intention of designs, e.g. I see that @mugamma's FMB PR has a test on range-based for loop, so I can immediately tell why he's writing the iterators earlier. This is not a huge issue here since this PR is mostly about math, though it'd be great if we can have more human readable tests :).
| float beta3; | ||
| float eta; | ||
|
|
||
| CUDA_CALLABLE __forceinline__ float blend(float t, float d) const { |
There was a problem hiding this comment.
I'd be nice to have definition of the methods in a blender.cu file :)
| std::vector<float> result(n); | ||
|
|
||
| CUDA_CHECK(cudaMalloc(&d_t, nbytes)); | ||
| CUDA_CHECK(cudaMalloc(&d_d, nbytes)); | ||
| CUDA_CHECK(cudaMalloc(&d_blended, nbytes)); | ||
| CUDA_CHECK(cudaMemcpy(d_t, t_vec.data(), nbytes, cudaMemcpyHostToDevice)); | ||
| CUDA_CHECK(cudaMemcpy(d_d, d_vec.data(), nbytes, cudaMemcpyHostToDevice)); |
There was a problem hiding this comment.
For readability: you might want to look into some of thrust's utilities (e.g. thrust::device_vector) to reduce the amount of manual memory management :).
tests/python_tests/test_blender.py
Outdated
| def create_blender_instance(kwargs: dict) -> FourParameterBlender: | ||
| """Helper to instantiate FourParameterBlender with the provided parameters.""" | ||
| return FourParameterBlender(kwargs["beta1"], kwargs["beta2"], kwargs["beta3"], kwargs["eta"]) |
There was a problem hiding this comment.
Given that this is just a single line & this function is only used in one place, how about moving this inline?
| # Check for finiteness (blend can in rare cases produce 0 or inf for extreme values) | ||
| assert np.isfinite(actual) | ||
|
|
||
| # INSERT_YOUR_CODE |
There was a problem hiding this comment.
just helps to make sure that the exp does not blow up, it was happening with some large param values
There was a problem hiding this comment.
oh I was referring to this random # INSERT_YOUR_CODE comment lol
| @pytest.mark.parametrize("blender_kwargs", BLENDER_TEST_CASES) | ||
| def test_blender_single_value(rng_seed: int, blender_kwargs: dict) -> None: |
There was a problem hiding this comment.
BTW: Pytest actually support parametrizing over multiple arguments. You might find this helpful if you want avoid carrying around a kwargs dict (I'm personally fine with leaving this here).
I have implemented two types of blender:
I have also added kwargs support for the
get_confidencemethod for the confidence bindings (minor QOL change)I have also removed the
rayandfmbargs fromforward.cu, for they are not needed. All we need is thet_iandd_i.NOTE: I have not implemented zero parameter blending. That one requires sorting all FMBs for every pixel, whic does not fit into our structure in
forward.cucurrently. I suggest we get to it if we want to (will triage it).