Skip to content
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

fetch the data on the table to add csv download button #648

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

RuTiO2le
Copy link
Contributor

@RuTiO2le RuTiO2le commented Oct 5, 2023

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.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

What does this implement/fix? Explain your changes.

  • fetch the data on the table to make a csv file
  • write a csv file
  • force to download the file

@RuTiO2le
Copy link
Contributor Author

RuTiO2le commented Oct 5, 2023

I am a participant in last Saturday's development sprint.
I'm sorry for the delay in PR because I forgot to click it.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #648 (44ad948) into main (0eae639) will decrease coverage by 0.41%.
Report is 31 commits behind head on main.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
- Coverage   62.45%   62.04%   -0.41%     
==========================================
  Files          35       35              
  Lines        2224     2266      +42     
==========================================
+ Hits         1389     1406      +17     
- Misses        835      860      +25     
Files Coverage Δ
optuna_dashboard/_app.py 53.42% <16.66%> (-2.48%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@c-bata
Copy link
Member

c-bata commented Oct 6, 2023

@RuTiO2le You don't have to be sorry 👍 Thank you for your contribution.

@keisuke-umezawa Could you review this PR?

@c-bata c-bata linked an issue Oct 6, 2023 that may be closed by this pull request
@keisuke-umezawa
Copy link
Member

@RuTiO2le
Sorry for being late. I will check it this week. Also, could you resolve ci for lint?

@keisuke-umezawa
Copy link
Member

keisuke-umezawa commented Oct 18, 2023

Current implementation:
スクリーンショット 2023-10-18 21 46 16

To make it more visible, can you follow something like this ui? https://mui.com/x/react-data-grid/export/

@keisuke-umezawa
Copy link
Member

keisuke-umezawa commented Oct 18, 2023

Example of multiple objective case:
スクリーンショット 2023-10-18 21 58 06
スクリーンショット 2023-10-18 21 58 33

  • Differences
    • csv file does not show all objectives
    • Parameter does not have Param.
    • State has TrialState.xxx

trials = get_trials(storage, study_id)

param_names = list(trials[0].params.keys())
union_user_attrs = list(trials[0].user_attrs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -447,6 +449,41 @@ def save_trial_note(study_id: int, trial_id: int) -> dict[str, Any]:
note.save_note_with_version(storage, study_id, trial_id, req_note_ver, req_note_body)
response.status = 204 # No content
return {}

@app.get("/csv/<study_id:int>")
def download_csv(study_id: int) -> BottleViewReturn:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow the original logic, there are some differences as follows:

  • Displaying user_attrs instead of union_user_attrs
  • Displaying params without considering union_search_space and intersection_search_space

@keisuke-umezawa
Copy link
Member

@RuTiO2le
I left some comments. Could you check it? If you would like to discuss it in Japanese, you can use discord.

@RuTiO2le
Copy link
Contributor Author

I'm sorry I missed notifications.
Thank you for your kind comments, I will check it this weekend.

@c-bata c-bata assigned c-bata and unassigned keisuke-umezawa Nov 30, 2023
This was referenced Dec 6, 2023
@HideakiImamura HideakiImamura merged commit d0be784 into optuna:main Dec 8, 2023
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download TrialTable as a csv file
4 participants