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

Improving documentation #477

Merged
merged 29 commits into from Oct 18, 2020
Merged

Improving documentation #477

merged 29 commits into from Oct 18, 2020

Conversation

lrouhier
Copy link
Contributor

@lrouhier lrouhier commented Oct 15, 2020

This PR aims at improving the documentation as a few section needs to be modified.

fix #473.
fix #475

to visualize: https://ivadomed.org/en/lr-fixing_documentation/

@lrouhier lrouhier marked this pull request as draft October 15, 2020 19:32
@jcohenadad
Copy link
Member

you could use this PR to also fix #475

@coveralls
Copy link

coveralls commented Oct 15, 2020

Pull Request Test Coverage Report for Build 311305440

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.721%

Totals Coverage Status
Change from base Build 310721790: 0.0%
Covered Lines: 5524
Relevant Lines: 6089

💛 - Coveralls

@lrouhier
Copy link
Contributor Author

I'll do that !

@lrouhier
Copy link
Contributor Author

I guess something like this would work.
Could we build this doc @jcohenadad ?
image

@jcohenadad
Copy link
Member

referring to #477 (comment):

  • could you find a green check not enclosed in a box? i vaguely remember seeing an ivadomed-like JMLR paper doing the same table, with nicer-looking checks/crosses
  • could you center the checks/cross
  • under uncertainty: could you split the two sub-columns?

@lrouhier
Copy link
Contributor Author

image
are these the checkmarks you were thinking about ?

@jcohenadad
Copy link
Member

are these the checkmarks you were thinking about ?

yup! these are nice. Do you have a red cross that is a bit less aggressive? (same style as the green check)

@lrouhier
Copy link
Contributor Author

like this one ?
image

@lrouhier
Copy link
Contributor Author

Just to be clear, this is actually HTML Unicode. I created two commands ( |yes| and |no|) that refer to HTML Unicode in purpose.rst. So the table needs to be filled with |yes| and |no| see here. It is a copy of the original table, because it ia WIP, it will replace the original one after approval.

@lrouhier lrouhier marked this pull request as ready for review October 16, 2020 03:08
@lrouhier
Copy link
Contributor Author

Do we have anything else to fix in the doc? otherwise I think it is fairly ready.

@jcohenadad
Copy link
Member

  • @lrouhier pls update wiht master
  • could you pls build this branch on RTD and add the link in your PR body so we can all review the rendered doc
    thx!

@lrouhier
Copy link
Contributor Author

@jcohenadad I think that you need to trigger the first build (or I need access code)

@jcohenadad
Copy link
Member

@jcohenadad I think that you need to trigger the first build (or I need access code)

i just triggered it-- @alexfoias can you add Lucas on the RTD so he can select the build in the future

@jcohenadad
Copy link
Member

jcohenadad commented Oct 16, 2020

hum, looks like there is a windows/macOS difference in the red cross:

image

Build #12118292
lr-fixing_documentation (c8ca60683cd5f0f3c8e51546a865c4a10da3d500)

@lrouhier
Copy link
Contributor Author

Check now! I forgot to push yesterday evening, I just built it again.

@jcohenadad
Copy link
Member

neat!
minor fixes:

  • ANTSPYNET: Kears --> Keras
  • ANTSPYNET: segmentation,clustering --> segmentation, clustering
  • column "Data dimension" --> be consistent with space after comma, and replace the "/" with comma

I'll also make suggestions for the "data" section

@@ -1,44 +1,59 @@
Models
======
Architectures
Copy link
Member

Choose a reason for hiding this comment

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

the file should be renamed architectures.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in
9eb7885

@jcohenadad
Copy link
Member

links to example config files are broken-- i'm fixing it

@jcohenadad
Copy link
Member

jcohenadad commented Oct 16, 2020

@lrouhier about 9eb7885: i've noticed this renaming broke the hard-coded path to models.rst in ivadomed's code.

also: index.rst needs to be updated

@lrouhier
Copy link
Contributor Author

It broke something in the code? I know it broke something in the pupose.rst (which I changed)
I have just changed what was needed in index.rst.

@jcohenadad
Copy link
Member

It broke something in the code? I know it broke something in the pupose.rst (which I changed)

my bad, looking good 👍

@jcohenadad
Copy link
Member

jcohenadad commented Oct 16, 2020

purpose table: niftytorch: Classificication --> Classification

also: use comma for consistency

@jcohenadad
Copy link
Member

purpose table: DL base library
what is the difference between "Keras (backend TF)" and "Tensorflow/Keras"?

@jcohenadad
Copy link
Member

purpose: table: https://github.com/neuropoly/ivado-medical-imaging --> https://github.com/ivadomed/ivadomed

@lrouhier
Copy link
Contributor Author

purpose table: DL base library
what is the difference between "Keras (backend TF)" and "Tensorflow/Keras"?

I don't think there is one. I think it just depends on who filled the row.

@jcohenadad
Copy link
Member

purpose table: DL base library
what is the difference between "Keras (backend TF)" and "Tensorflow/Keras"?

I don't think there is one. I think it just depends on who filled the row.

so it should be changed to be consistent across cells

@lrouhier
Copy link
Contributor Author

I modified the table so it is consistent. I will check for typo and/or similar cases after compilation.

@lrouhier lrouhier added this to the next-release milestone Oct 16, 2020
@lrouhier lrouhier added the documentation category: readthedocs or user doc label Oct 16, 2020
@charleygros charleygros merged commit dde5748 into master Oct 18, 2020
@charleygros charleygros deleted the lr/fixing_documentation branch October 18, 2020 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation category: readthedocs or user doc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change yes/no by ✅ / ❌ Documentation: Refactor the getting started section
4 participants