DeepRegNet / DeepReg

Medical image registration using deep learning
Apache License 2.0
568 stars 77 forks source link

Documentation review - pre-r1 #322

Closed YipengHu closed 4 years ago

YipengHu commented 4 years ago

Issue description

Review the documentation. Due 27th (sorry again)


@s-sd https://github.com/DeepRegNet/DeepReg/tree/master/docs/source/tutorial

@acasamitjana https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/dataset_loader.md

@ebonmati https://github.com/DeepRegNet/DeepReg/tree/master/docs/source/getting_started https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/configuration.md https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/cli.md https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/experimental.rst https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/exp_label_sampling.md


Type of Issue

How to review

shannonxtreme commented 4 years ago

Hey guys, I'm a technical writer looking for projects to write docs for to expand my own portfolio. If you want, I could help out with this project!

YipengHu commented 4 years ago

Hey guys, I'm a technical writer looking for projects to write docs for to expand my own portfolio. If you want, I could help out with this project!

@shannonxtreme Everyone is welcome to contribute of course! Please follow the contributing guidelines and relevant documents, and raise an issue if anything not clear! https://deepreg.readthedocs.io/en/latest/contributing/issue.html

shannonxtreme commented 4 years ago

In install.rst one suggestion I have is to make external links open in new tabs by default. Applies to all docs, I suppose

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/getting_started/install.rst --> "Dependent external libraries include those provide core IO functionalities and other standard processing tools. The dependencies for this package are managed by pip": @agrimwood @YipengHu Do you think this sentence is grammatically correct? should it be " those that provide" instead?

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/getting_started/install.rst --> "The dependencies for this package are managed by pip". Does "this package" refer to DeepReg? it is not clear and I suggest to state that DeepReg instead of "this package".

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/configuration.md --> all good

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/cli.md --> "More configuration can be specified in the configuration file": @agrimwood is this sentence grammatically correct? I am not sure...

shannonxtreme commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/getting_started/install.rst --> "Dependent external libraries include those provide core IO functionalities and other standard processing tools. The dependencies for this package are managed by pip": @agrimwood @YipengHu Do you think this sentence is grammatically correct? should it be " those that provide" instead?

Can't believe I missed that. Yes, those that provide is correct.

shannonxtreme commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/getting_started/install.rst --> "The dependencies for this package are managed by pip". Does "this package" refer to DeepReg? it is not clear and I suggest to state that DeepReg instead of "this package".

Reading through it as an absolute newbie to python and packaging and all that fun stuff, it was clear to me as- is. DeepReg is the only package mentioned by name in that paragraph, so naming it twice would be more repetitive than anything.

agrimwood commented 4 years ago

Sorry, I just read your comment @shannonxtreme. I have changed the text in #336. You're right that the only package referred to is DeepReg. Even so, I have changed for clarity, because the term could equally be used to reference the dependencies (they are also packages). In this case it's debatable, but I have chosen repetition over ambiguity.

shannonxtreme commented 4 years ago

Sorry, I just read your comment @shannonxtreme. I have changed the text in #336. You're right that the only package referred to is DeepReg. Even so, I have changed for clarity, because the term could equally be used to reference the dependencies (they are also packages). In this case it's debatable, but I have chosen repetition over ambiguity.

Sounds good! Thanks for following up.

shannonxtreme commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/cli.md --> "More configuration can be specified in the configuration file": @agrimwood is this sentence grammatically correct? I am not sure...

It's an awkward sentence. "Additional options can be configured in filename.yml" or "Additional options can be specified in the configuration file."

If the config file has a static filename, mention the name. If not, use the latter option and link it to the corresponding doc. The last sentence "See configuration file for further details" can be removed in the second case.

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/experimental.rst --> Broken link in [new issue]

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/experimental.rst --> Broken link in "Label Sampling"

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/exp_label_sampling.md --> The narrative of the first paragraph could be improved for a better flow. "When each image has multiple labels, e.g. segmentations of different organs in a CT image. For each sampled image pair, one label pair is randomly sampled. This is default for training."

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/blob/master/docs/source/docs/exp_label_sampling.md --> in "Consistent label pairs" the second paragraph has a series of '-' that look like the paragraph is either not formatted correctly or the '-' need to be removed...

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/tree/master/docs/source/getting_started --> WSL + master branch links are broken

ebonmati commented 4 years ago

https://github.com/DeepRegNet/DeepReg/tree/master/docs/source/getting_started --> WSL + master branch links are broken

Actually, they are not broken. In Chrome if I open the link in a new tab, it works, but if I open the link directly it does not open (github refused to connect). Anyone else experiencing the same?

shannonxtreme commented 4 years ago

@ebonmati I'll verify as well soon! Also, #337 is an ongoing PR with some edits to the experimental section that you just reviewed.

Also folks, I'm not usually this careless. I seem to have let quite a few small issues past my review. I'll try and do better!

ebonmati commented 4 years ago

@ebonmati I'll verify as well soon! Also, #337 is an ongoing PR with some edits to the experimental section that you just reviewed.

Also folks, I'm not usually this careless. I seem to have let quite a few small issues past my review. I'll try and do better!

I think this may be related to #324 too.

shannonxtreme commented 4 years ago

In tutorial/cross_val.md, the first link to docs/dataset_loader.md is broken, but I don't know what's wrong with it. The path in the markdown file is correct but it breaks when built. Is this true for you all?

Link: https://deepreg.readthedocs.io/en/latest/tutorial/cross_val.html

Edit: all links on that page appear to not work for me.