Fix a bug of smart filter in TrialList by simplifying React states #1172
Conversation
|
@toshihikoyanase Could you review this PR? |
There was a problem hiding this comment.
Thank you for your PR. I confirmed that it significantly reduced the number of filtered-trial renderings.
I was expecting the results from filtering by trial status and LLM prompts to be merged together, but that's not the case in my environment.
Could you explain how these should be integrated?
Case 1: state filtering, then LLM filtering
- exclude the Complete status in TrialState.
- apply a limit by trial number using LLM (i.e.,
(t) => t.number < 3in JavaScript). - restore Complete status in TrialState
Expected: display the trial #0 to trial #2.
Actual: nothing appears.
Case 2: LLM filtering, then state filtering
- filter the trials by their numbers using LLM.
- exclude completed trials.
Expected: No trials displayed.
Actual: display the trial #0 to trial #2.
c-bata
force-pushed
the
fix-trial-list-smart-filter
branch
from
4b3f8bb to
2203277
Compare
|
@toshihikoyanase Thank you for your review! You are completely correct. I fixed that bug in 2203277. Please take a look again 🙇 |
|
@c-bata Thank you for your update. I confirmed that the issue regarding the merge of filtering results has been resolved. Next, I'll check the change line by line. |
toshihikoyanase
left a comment
toshihikoyanase
left a comment
There was a problem hiding this comment.
The change looks great to me. I added just a minor comment.
|
|
||
| const trials = useMemo(() => { | ||
| // Start with LLM filtered trials if available, otherwise all trials | ||
| const baseTrials = llmFilteredTrials ?? allTrials |
There was a problem hiding this comment.
Not a strong opinion, but baseTrials reminds me of a base class for the Trial object.
Since we're applying filters step by step here, could we name the filter results something else? Given that these are local variables in small functions, it doesn't matter too much. Just to confirm, I'd like to ask for your opinion.
There was a problem hiding this comment.
Sounds make sense. I renamed it to trialsBeforeStateFilter in 597ff6c. Let me know if you have any preference or other suggestions!
There was a problem hiding this comment.
Thank you for your update! Looks reasonable.
2 participants
Contributor License Agreement
This repository (
optuna-dashboard) and Goptuna share common code.This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.
Reference Issues/PRs
None
What does this implement/fix? Explain your changes.
Fixes a bug of smart filtering in TrialList component.