[WIP] validator and refactor - #1239
Open
sergey-yaroslavtsev wants to merge 2 commits into
Open
Conversation
sergey-yaroslavtsev
changed the title
woutdenolf
requested changes
Aug 6, 2026
woutdenolf
left a comment
woutdenolf
left a comment
Collaborator
There was a problem hiding this comment.
Looks good overall. Some small suggestions.
Comment on lines
169
to
185
| def _slot(self, button): | ||
| if hasattr(button, "text"): | ||
| # received a button | ||
| pass | ||
| else: | ||
| # received an integer | ||
| button = self.buttonGroup.button(button) | ||
| if "first" in safe_str(button.text()).lower(): | ||
| self._stackIndex = 0 | ||
| text = safe_str(button.text()).lower() | ||
| if text.startswith("no"): | ||
| self._noMca = True | ||
| self._stackIndex = -1 | ||
| elif "first" in text: | ||
| self._noMca = False | ||
| self._stackIndex = 0 | ||
| else: | ||
| self._noMca = False | ||
| self._stackIndex = -1 |
Collaborator
There was a problem hiding this comment.
I propose a different slot for each of the three buttons.
| if _channels.size == self.data.shape[self.info["McaIndex"]]: | ||
| self.x = [_channels] | ||
| else: | ||
| # if metadata do not match the data, ignore them |
Collaborator
There was a problem hiding this comment.
Suggested change
| # if metadata do not match the data, ignore them | |
| # if metadata does not match the data, ignore them |
| # Cancel return user to Wizard | ||
| return None | ||
|
|
||
| def _effectiveShape(self, selection, rawShape): |
Collaborator
There was a problem hiding this comment.
Suggested change
| def _effectiveShape(self, selection, rawShape): | |
| @staticmethod | |
| def _effectiveShape(selection, rawShape): |
| if ndim > 2: | ||
| self.showMessage( | ||
| "Three dimensions could not represent an image. " | ||
| "'No 1D data' support only 2D and 1D datasets. " |
Collaborator
There was a problem hiding this comment.
Suggested change
| "'No 1D data' support only 2D and 1D datasets. " | |
| "'No 1D data' supports only 2D and 1D datasets. " |
| else: | ||
| if ndim < 2: | ||
| self.showMessage( | ||
| "A 1D dataset could not represent an image and channels. " |
Collaborator
There was a problem hiding this comment.
Suggested change
| "A 1D dataset could not represent an image and channels. " | |
| "A 1D dataset could not represent an image with channels. " |
| if ndim < 2: | ||
| self.showMessage( | ||
| "A 1D dataset could not represent an image and channels. " | ||
| "Use 'No 1D data (image)' in case there is no channel.") |
Collaborator
There was a problem hiding this comment.
Suggested change
| "Use 'No 1D data (image)' in case there is no channel.") | |
| "Use 'No 1D data (image)' in case there are no channels.") |
Comment on lines
+548
to
+549
| noMca = selection.get('noMca') | ||
| scatter = selection['scatter'] |
Collaborator
There was a problem hiding this comment.
In several places you use .get('noMca') instead of ['noMca']. We don't do this for 'scatter' for example. What is the difference?
Comment on lines
419
to
+422
| selection['scatter'] = self._scatterCheckBox.isChecked() | ||
| selection['allowPadding'] = False | ||
| selection['squeeze'] = False | ||
| selection['noMca'] = self.stackIndexWidget._noMca |
Collaborator
There was a problem hiding this comment.
No need for .get() on these keys, they are always defined.
Comment on lines
+258
to
+260
| if self._stack.info.get("Squeeze", False) and\ | ||
| isinstance(self._stack.data, numpy.ndarray): | ||
| oldshape = self._stack.data.shape | ||
| dialog = ImageShapeDialog(self, shape=oldshape[0:2]) | ||
| if self._squeezeSingletonStack(): |
Collaborator
There was a problem hiding this comment.
Is would check for "Squeeze" and numpy.ndarray inside _squeezeSingletonStack.
I was confused why in _squeezeSingletonStack we set info["Squeeze"] = False. If the check is inside _squeezeSingletonStack that would be clear.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Close #1238
positive:
many validators were added so the user have a info about what is wrong and even suggestions what was selected wrong. + it sends back to Wizard instead of closing it with error.
clear use of "1D data" position. Now there is no hidden (from UI perspective) possibility to open 2D image. To open images as stack "No 1D data" can be selected.
singletons are dropped or kept automatically if possible; in cases where it is not obvious the user is asked what to do with them.
if axis are bigger than dataset - padding is proposed (or cancel to reselect).
other changes:
a) pure singleton is not acceptable - since it makes no sense and would require a special handler.
b) order of axes matters - it is tipped in selection - it was done because before if the number of channels was equal to the size of dimension - it was unclear what will happen.
dimension validator gives:
then there is next validator based on selected axes. The full table of every possible outcome will be quite big.
NOTES:
Some things could be not obvious on first place but there is a logic - for example:
if dataset have singletons and "No 1D data" is selected. Singletons are dropped automatically because having them would only restrict dimensions - while without them user can reshape - and default option is the one as if they do exist (it is for free).
I used some AI generated tests to verify obvious things but i will test more things manually - that is why it is [WIP].
Feel free to suggest different logic. I am also fine if you would sat that this PR is bad idea - but then please provide an idea(s) to fix current state.