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

add BC task script in readme #600

Merged
merged 4 commits into from
Jan 18, 2023
Merged

add BC task script in readme #600

merged 4 commits into from
Jan 18, 2023

Conversation

rnyak
Copy link
Contributor

@rnyak rnyak commented Jan 17, 2023

No description provided.

@rnyak rnyak requested a review from sararb January 17, 2023 17:36
@rnyak rnyak added the documentation Improvements or additions to documentation label Jan 17, 2023
@github-actions
Copy link

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mikemckiernan
Copy link
Member

mikemckiernan commented Jan 17, 2023

I gave it a whirl--including some creative writing on the parameters to the class:

I drafted the change as a make-believe PR in my fork. If you like the changes, I can add the commit to this PR. It seemed too presumptuous to add a commit to your PR without checking.

@rnyak
Copy link
Contributor Author

rnyak commented Jan 17, 2023

I gave it a whirl--including some creative writing on the parameters to the class:

I drafted the change as a make-believe PR in my fork. If you like the changes, I can add the commit to this PR. It seemed too presumptuous to add a commit to your PR without checking.

@mikemckiernan it sounds fine to me but we also change the masking to None in the input_module for BC task. so that'd be also different than the next-item prediction task. I understand your point. you think the code snippets are very similar, and big chunk is repeated twice.. But I'd say better to repeat it than defining it insufficiently. what do you think?

@mikemckiernan
Copy link
Member

mikemckiernan commented Jan 18, 2023

@mikemckiernan it sounds fine to me but we also change the masking to None in the input_module for BC task. so that'd be also different than the next-item prediction task. I understand your point. you think the code snippets are very similar, and big chunk is repeated twice.. But I'd say better to repeat it than defining it insufficiently. what do you think?

I don't know if I'll persuade you to my thinking, but I'll try. With so much similarity, the reader is using brain power to detect what is different (and wondering why two tasks are so overwhelmingly similar) than focusing on learning the concept--learning how to write the code to run a particular type of prediction task, classification, and so on.

I think it'd be better to include more of the sample code in the API reference section, as long as it is typical that most folks will want to specify those blocks. I made the proposed change in the API desc.

My feeling is that we can't provide end-to-end examples for all models. The introductory material in the README should show common, typical, or simple examples. For less common models, code, and so on, the API reference is pretty ideal--can anyone really argue with finding sample code about binary classification anywhere other than the API ref for the binary classification class?

Copy link
Contributor

@bschifferer bschifferer left a comment

Choose a reason for hiding this comment

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

do we not need the schema.pb file in the examples?

@rnyak
Copy link
Contributor Author

rnyak commented Jan 18, 2023

do we not need the schema.pb file in the examples?

@bschifferer thanks for the comment. for the getting-started this schema file is not required, it is generated from NVT and read in then. We kept it there due to unit test, now we changed the unit test and it is run via testbook, we dont need this file either. This file creates confusion for the users so let's remove it :)

- Copy code from README up to the point
  of getting the model object.
@mikemckiernan mikemckiernan merged commit 0a24320 into main Jan 18, 2023
@mikemckiernan mikemckiernan deleted the add_bc_to_readme branch January 18, 2023 15:53
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants