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

Updates edit column menu to support hparam columns #6738

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

hoonji
Copy link
Member

@hoonji hoonji commented Jan 29, 2024

Motivation for features / changes

#6737 will allow hiding hparam columns from the scalar card tables, but there is currently no way to show them again. Toggling standard columns is currently being done in the edit columns menu - this is a natural place to also allow hparam column toggling.

Technical description of changes

  • Creates a new section in the edit columns menu for showing hparam columns. The column is only shown when the hparam flag is enabled and is the same across single and range selection tables.
  • Implements hparam column add, reorder, toggle from the edit columns menu.

Screenshots of UI changes (or N/A)

New hparams section:
edit_hparam_1
Adding hparams:
edit_hparam_2

Detailed steps to verify changes work correctly (as executed by you)

  • Manually tested add/reorder/sort/filter/remove/hide in runs table, scalar table, and edit column menu.

@hoonji hoonji requested a review from rileyajones January 29, 2024 14:13
@rileyajones
Copy link
Contributor

rileyajones commented Jan 29, 2024

I'm not sure this approach to adding hparam columns to the scalar card actually scales. The hparams data project is about to launch and experiments will have 1000s of hparams. Thus the importance of the search feature we built into the runs table columns selector. Rendering out 1000s of line items in the column selector there is going to get out of control quickly.

I was under the impression that the plan for adding hparam columns to the scalar card data table was to get rid of the current affordance and replace it with the newer ui.

@hoonji
Copy link
Member Author

hoonji commented Jan 30, 2024

I'm not sure this approach to adding hparam columns to the scalar card actually scales. The hparams data project is about to launch and experiments will have 1000s of hparams. Thus the importance of the search feature we built into the runs table columns selector. Rendering out 1000s of line items in the column selector there is going to get out of control quickly.

I was under the impression that the plan for adding hparam columns to the scalar card data table was to get rid of the current affordance and replace it with the newer ui.

@rileyajones I suspect there may be a misunderstanding here somewhere -

To establish some ground truths:

  • all hparam column adds must be done via the searchable column selector, whether it is accessed via the column context menus (Insert column Right/Left) or via the + button on the tables or in the edit columns menu
  • the column selector can easily support ~5000 rows and starts lagging a bit on O(10000) rows. If we need to improve scroll performance, we can leverage things like virtual scroll

So I'm not exactly sure what the point of concern is here - to clarify, the hparam list above is only going to show the "added" columns, not all columns. Also, we can't get rid of this menu because it's the only UI that allows us to re-enable columns.

Copy link
Contributor

@rileyajones rileyajones left a comment

Choose a reason for hiding this comment

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

This is quite different from the UI that we had discussed. It was our plan to remove the scalar_column_editor all together and control all scalar card columns via the newer UI.

We have a meeting tomorrow, lets discuss this in detail then.

Base automatically changed from hparam_scalar_table_hparams to master February 6, 2024 12:42
@hoonji
Copy link
Member Author

hoonji commented Feb 13, 2024

@rileyajones To clarify next steps, IIUC we eventually want to completely eliminate the "edit table columns" and consolidate all add/remove/sort behavior to use only context menu actions.

Before this can be done, we first have to allow adding standard columns using the "select columns" affordance - as currently the only way to re-show standard columns (or show columns that are disabled by default) is via the "edit table columns" menu. The select columns affordance will be upgraded as described in the next phase - I'll keep the "edit table columns" menu alive 'til then.

(Note that this is just an FYI comment - this PR we can probably trash)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants