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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Evaluate segmentation in infer neurons task #8221
Evaluate segmentation in infer neurons task #8221
Changes from 5 commits
b2d9627
ac88d7d
0fa0cfd
3258794
785ddff
bda408e
92b6169
67a55ff
63bade2
870b589
581ed00
2e2d915
9b713b7
d76197b
c5f82f1
55cd8ac
5c7c707
6055346
97cd24c
db3e4d1
4ee1467
213d110
23a95fe
1a7c1eb
282786d
7ca5d48
5f07ad6
8ad0409
9d6572c
e2a045e
e819687
be88173
9be4af9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't know what kind of evaluation would be performed in case
doEvaluation
is set to true. (As I do not have work with voxelytics regularly) Therefore, I would prefer to make the name more explicitThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same renaming here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable renaming from above and adding spaces between param definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And so on 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add parameter validation for evaluation settings.
The code should validate optional parameters before appending them to prevent potential
undefined
values from being sent to the API.📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its ok to do this suggestion, but this isn't a must imo.
The backend should at last realize some params are missing (see my comment above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelBuessemeyer, understood. If the backend handles missing parameters, additional validation on the frontend is indeed optional.
✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also rename the name path everywhere from
"evaluationSettings"
to"splitMergerEvaluationSettings"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as this is within the
CollapsibleSplitMergerEvaluationSettings
component, this might already be guessable / rather strait forward. So please only adapt if you want to :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty
Form.Item
without a child componentThe
Form.Item
at line 584 does not contain any input or form control as its child.Form.Item
is intended to wrap form controls like inputs, checkboxes, or selectors. An emptyForm.Item
may lead to unexpected behavior and should be avoided. Please ensure that you include the appropriate form control within thisForm.Item
.Apply this diff to address the issue:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jup, the AI is correct here. But it seems you simply want the form data to always have
useAnnotation=true
in case an evaluation is done, correct? In that case, I'd suggest to make the form item hidden:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelBuessemeyer, thank you for the clarification. Yes, making the
Form.Item
hidden withinitialValue={true}
is a good solution to ensure thatuseAnnotation
is always set totrue
when evaluation is done, without rendering an empty form item. I've noted this for future reviews.✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Potential issue with
Collapse
component'sonChange
handlerThe
onChange
handler for theCollapse
component receives the active key(s) as arguments. Currently, the code togglesisActive
using!isActive
without considering these arguments, which might lead to synchronization issues between theCollapse
state and theisActive
state. It is recommended to use the providedkey
parameter to update theisActive
state accurately.Apply this diff to improve the implementation:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont see the issue here.
I'd ignore that @cdfhalle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect usage of
tooltip
prop inForm.Item
componentsThe
Form.Item
component from Ant Design does not support atooltip
prop directly. To display a tooltip for the label, you should wrap the label content with theTooltip
component. This ensures that the tooltip behaves as expected and follows the Ant Design specifications.Apply this diff to fix the issue:
Apply similar changes to the other
Form.Item
components with tooltips.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but you are wrong here AI :)
The docs state that there is a tooltip prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say to also ignore that @cdfhalle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just guessing here: The split merger evaluation requires an annotation to work, doesn't it? And more specific a skeleton annotation is required, isn't it?
If that's so please only set the
jobSpecificInputFields
toCollapsibleEvaluationSettings
if the user currently has an opened annotation with a skeleton annotation.You can retrieve whether there is a skeleton tracing via: