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

redefine model output representation to be two columns #202

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

micokoch
Copy link
Contributor

Addressing issue #201

@micokoch micokoch added the documentation Improvements or additions to documentation label Oct 23, 2024
@micokoch micokoch requested a review from elray1 October 23, 2024 15:47
@micokoch micokoch self-assigned this Oct 23, 2024
@micokoch micokoch linked an issue Oct 23, 2024 that may be closed by this pull request
@zkamvar
Copy link
Member

zkamvar commented Oct 23, 2024

Ah crap. This was part of my changes in #197 🙈

@zkamvar zkamvar changed the title addressing #201 redefine model output representation to be two columns Oct 23, 2024
Copy link
Contributor

@LucieContamin LucieContamin left a comment

Choose a reason for hiding this comment

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

Looks good to me! thanks!

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

A couple of changes to make it consistent.

There's also one change that I cannot suggest inline, but line 81 needs to be changed to reflect that the model output representation is made up of two columns and is followed by the model output value column.

docs/source/user-guide/model-output.md Outdated Show resolved Hide resolved
docs/source/user-guide/model-output.md Outdated Show resolved Hide resolved
@zkamvar zkamvar self-requested a review October 24, 2024 15:29
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

One more change from my previous review that I cannot suggest on the PR itself:

line 81 needs to be changed to reflect that the model output representation is made up of two columns and is followed by the model output value column.

@micokoch micokoch removed the request for review from elray1 October 24, 2024 15:43
@micokoch
Copy link
Contributor Author

Hi @zkamvar ! Thanks for looking at the page more carefully. I accepted the changes you suggested and edited line 81. I hope that works for you.

@micokoch micokoch requested a review from zkamvar October 24, 2024 15:56
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

Looks good! One more change for clarity.

@@ -78,7 +78,7 @@ Much of the material in this section has been excerpted or adapted from the [hub

_Model outputs are a specially formatted tabular representation of predictions._
Each row corresponds to a unique prediction, and each column provides information about what is being predicted, its scope, and its value.
Per hubverse convention, **there are two groups of columns**[^model-id], each group serving a specific purpose: (1) the **"task ID"** columns provide details about what is being predicted, and (2) the three **"model output representation"** columns specifies the type of prediction, identifying information about that prediction, and the value of the prediction.
Per hubverse convention, **there are two groups of columns and one column for model output**[^model-id]. Each group of columns serves a specific purpose: (1) the **"task ID"** columns provide details about what is being predicted, and (2) the two **"model output representation"** columns specify the type of prediction, and identifying information about that prediction. Finally, (3) the **value** column provides the model output of the prediction.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Per hubverse convention, **there are two groups of columns and one column for model output**[^model-id]. Each group of columns serves a specific purpose: (1) the **"task ID"** columns provide details about what is being predicted, and (2) the two **"model output representation"** columns specify the type of prediction, and identifying information about that prediction. Finally, (3) the **value** column provides the model output of the prediction.
Per hubverse convention, **there are two groups of columns for model output**[^model-id] followed by a single column for values. Each group of columns serves a specific purpose: (1) the **"task ID"** columns provide details about what is being predicted, and (2) the two **"model output representation"** columns specify the type of prediction and identifying information about that prediction. Finally, (3) the **value** column provides the model output of the prediction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding of #201, the column for model output is the column for values (the others are "metadata about the representation of that output"). I phrased it differently in my most recent commit.

@micokoch micokoch requested a review from zkamvar October 24, 2024 16:23
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your patience!

@micokoch micokoch merged commit 7229c21 into hubverse-org:main Oct 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

clean up description of groups of columns in model output
3 participants