search

PerkLab / SlicerSandbox

Collection of utilities that are not polished implementations but can be useful to users
BSD 2-Clause "Simplified" License
67 stars 24 forks source link

Add CharacterizeLinearTransform module #14

Closed mikebind closed 2 years ago

mikebind commented 2 years ago

Finally following up on this suggestion: https://discourse.slicer.org/t/rotational-transformation-matrix-surface-registration-of-models/23680/5 . Please let me know if any other changes or modifications are needed, thanks!

mikebind commented 2 years ago

Also worked through my remaining issues for the StitchVolumes module referenced here: https://discourse.slicer.org/t/combine-two-volumes-into-one-using-python/23152/6

Since the first pull request (from just a few hours ago) hasn't been merged, this PR contains the changes for CharacterizeLinearTransform and for StitchVolumes. If you want me to separate them somehow so they can be submitted separately please let me know.

mikebind commented 2 years ago

The changes contain a merge commit. Please rebase to remove it.

I'm not sure how to do this. I see the git rebase command, and I think you are asking that I move all my commits to start from the current Sandbox state (instead of branching off from an earlier point), and also to just remove the merge commit (because it won't be relevant anymore)). Do I do that on my fork (I think I can probably figure out how to do that) and then click "Contribute" again on my github fork to add those changes to this PR (I think that's what happens if I click that button again)?

mikebind commented 2 years ago

OK, I tried the rebase and think I have successfully removed the merge commit. Is there anything else I need to do for that issue? I will work through all the other suggested modifications now.

mikebind commented 2 years ago

I have addressed all the comments raised in the review by @lassoan (thank you!) There are a few remaining issues and feature requests which I will attempt to address over time. I am not sure what the next step is, but I am going to try clicking to re-request a review because that seems logical :)

mikebind commented 2 years ago

@lassoan, just bumping this. Please let me know if there is anything which needs to be addressed before merging.

lassoan commented 2 years ago

Sorry for the delay, I was travelling and many tasks piled up. I've merged your pull request now. Thanks a lot for your contributions!