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

docs: fix typos etc #666

Merged
merged 11 commits into from
Jan 26, 2023
Merged

docs: fix typos etc #666

merged 11 commits into from
Jan 26, 2023

Conversation

guenthermi
Copy link
Member

@guenthermi guenthermi commented Jan 24, 2023

Fix typos and duplicate text in docs

I removed the duplicate part in the documentation and fixed a couple of typos and minor wrong formulations.

  • This PR references an open issue
  • I have added a line about this change to CHANGELOG

docs/advanced-topics/negative-mining.md Outdated Show resolved Hide resolved
docs/get-started/how-it-works.md Outdated Show resolved Hide resolved
docs/notebooks/text_to_image.ipynb Outdated Show resolved Hide resolved
docs/notebooks/text_to_image.md Outdated Show resolved Hide resolved
Copy link
Contributor

@LMMilliken LMMilliken left a comment

Choose a reason for hiding this comment

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

This example for a CSV file contains a text item with a comma, an explanation for dealing with this is included below but maybe enclose this line with "s in case people just copy-paste without reading ahead

image

@guenthermi
Copy link
Member Author

This example for a CSV file contains a text item with a comma, maybe include a hint explaining how to correctly format items containing commas for CSV files.

image

In this case it should work because it uses tabs as delimiters right? How to handle commas is already explained here: https://github.com/jina-ai/finetuner/pull/666/files#diff-7396eec4b8628a7cf7461d430efcbdd11c0dc7a2cd1e6864b893817f6cf1a0caR95-R108

guenthermi and others added 2 commits January 25, 2023 08:58
Co-authored-by: George Mastrapas <32414777+gmastrapas@users.noreply.github.com>
@github-actions github-actions bot added size/l and removed size/m labels Jan 25, 2023
Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

LGTM!

@guenthermi
Copy link
Member Author

It seems like the tabs in the tsv examples have been spaces. This is caused, because many code editors automatically replace tabs with spaces, even in markdown. So if users copied this csv file they did not work. To prevent this, I replaced the tabs with commas. In this way it does not look as nice as before, but the at least it is correct.

Copy link
Contributor

@LMMilliken LMMilliken left a comment

Choose a reason for hiding this comment

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

Nice improvements overall

Comment on lines 95 to 108

```{important}
If a text field contains commas, it breaks the CSV format since it is interpreted as spanning over multiple columns.
In this case, please enclose the field in double quotes, such as `field1,"field, 2"`.
```

We support the following dialects of CSV:

+ `excel` use `,` as delimiter and `\r\n` as lineterminator.
+ `excel-tab` use `\t` as delimiter and `\r\n` as lineterminator.
+ `unix` use `,` as delimiter and `\n` as lineterminator.

```{warning}
Please remove/replace comma in your data fields if you are using a comma `,` as a delimiter.
Please remove/replace commas in your data fields if you are using a comma `,` as a delimiter.
Copy link
Contributor

Choose a reason for hiding this comment

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

These two hints are saying conflicting things about dealing with commas in CSVs, I think it would be best to remove the warning

@bwanglzu bwanglzu changed the title Docs fix typos etc docs: fix typos etc Jan 25, 2023
@guenthermi guenthermi self-assigned this Jan 25, 2023
Co-authored-by: Scott Martens <70647348+scott-martens@users.noreply.github.com>
@github-actions
Copy link

📝 Docs are deployed on https://ft-docs-fix-typos-etc--jina-docs.netlify.app 🎉

@guenthermi guenthermi merged commit 164f180 into main Jan 26, 2023
@guenthermi guenthermi deleted the docs-fix-typos-etc branch January 26, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants