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

Reviewed text and descriptions #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

matichorvat
Copy link

Liking things, Andraz will have a PR with some additions/changes to the code. I reviewed the descriptions.

I like the look of the pipeline diagram a lot, but there is some ambiguity there that could confuse readers. If you have access to the original files, I'd suggest the following changes:

  • the arrows from both 'parameters' boxes should go to 'Transform' instead of 'Output'
  • I'd rename 'Classifying features' into 'Test features' so that they correspond to 'Training features'
  • the fact that both train and test goes through the same pipeline and are joined into one arrow at the transformer output means that it is not clear that the classifer is fitted only on the transformed training data and not on the transformed test data. In order to make that clear, I think you'd effectively need two pipelines, one for training (both fit+transform) and one for testing (just transform), where the latter uses the parameters of the training one.

What do you think?

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.

1 participant