Skip to content

Reviewed text and descriptions#2

Open
matichorvat wants to merge 1 commit into
masterfrom
review-matic
Open

Reviewed text and descriptions#2
matichorvat wants to merge 1 commit into
masterfrom
review-matic

Conversation

@matichorvat

Copy link
Copy Markdown

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