Visualize feature point alignment between the base image and the aligned image #86
Conversation
stefsmeets
left a comment
stefsmeets
left a comment
There was a problem hiding this comment.
Nice work, I left some comments in the code.
My main concern is that this is now the third different implementation of the 'detect and extract' pattern. We have src/unraphael/feature.py (skimage), src/unraphael/dash/align.py:feature_align (openCV) and now this function (skimage).
What is troubling me is that the alignment itself is using the OpenCV function, but that the display is using the skimage version without using the same parameters (e.g. the user can pick the max_ratio). And also, it uses the aligned images to find the feature points? This makes the visualization completely arbitrary, because it shows something completely different. I think the visualization should show the points and source images that are actually used in the alignment.
| def feature_based_alignment_visual( | ||
| base_image: np.ndarray, |
There was a problem hiding this comment.
I suggest protecting this function by requiring keyword arguments:
| def feature_based_alignment_visual( | |
| base_image: np.ndarray, | |
| def feature_based_alignment_visual( | |
| *, | |
| base_image: np.ndarray, |
| Returns | ||
| ------- | ||
| None | ||
| Displays a plot showing the feature-based alignment |
There was a problem hiding this comment.
This is not necessary, but please update the function name/documentation that this will plot the aligment
| Returns | |
| ------- | |
| None | |
| Displays a plot showing the feature-based alignment |
| else: | ||
| st.error('Unsupported method selected.') | ||
| return |
There was a problem hiding this comment.
Consider raising a ValueError instead of silently failing.
| ) | ||
|
|
||
| return res | ||
| return res, align_method, motion_model |
There was a problem hiding this comment.
Can we refactor this in a way that avoids returning a tuple? This makes the code difficult to maintain.
| display_in_grayscale = ( | ||
| col2.radio('Display images in:', ['Grayscale', 'Original color)']) | ||
| == 'Grayscale' | ||
| ) | ||
| # Slider to select max_ratio | ||
| max_ratio = col2.slider('Max ratio for descriptor matching', 0.5, 0.8, 0.6, 0.01) | ||
|
|
||
| st.write('') | ||
| st.write('') | ||
| st.subheader(f'Feature-based alignment visualization using {motion_model}') | ||
| st.write('') |
There was a problem hiding this comment.
I think this should be in the widget.
| with col1: | ||
| st.button('⏮️ Previous', on_click=previous_image) | ||
| with col3: | ||
| st.button('Next ⏭️', on_click=next_image) |
There was a problem hiding this comment.
Nice to have: sync these buttons with those from the comparison itself.
| def next_image(): | ||
| st.session_state.image_index = (st.session_state.image_index + 1) % len(images) | ||
|
|
||
| def previous_image(): | ||
| st.session_state.image_index = (st.session_state.image_index - 1) % len(images) |
There was a problem hiding this comment.
Nice to have: We use this pattern now in several places. Can we refactor/generalize it so that we avoid this code duplication everywhere?
| def display_current_image(): | ||
| current_image = images[st.session_state.image_index] | ||
| feature_based_alignment_visual( | ||
| base_image=base_image.data, | ||
| target_image=current_image.data, | ||
| method=method, | ||
| base_image_name=base_image.name, | ||
| target_image_name=current_image.name, | ||
| display_in_grayscale=display_in_grayscale, | ||
| max_ratio=max_ratio, | ||
| ) |
There was a problem hiding this comment.
Does this need to be a function?
This PR adds the option to the alignment page to visualize the feature point alignment between the base image and the aligned image. This is helpful because it allows to evaluate whether the feature based alignment is suited for the alignment of the data used.