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

Refactor documentation #171

Merged
merged 34 commits into from
Aug 5, 2021
Merged

Conversation

14thibea
Copy link
Contributor

@14thibea 14thibea commented Jul 20, 2021

Updates on MAPS structure coming soon...

Some modifications were added to preserve previous functionalities:

  1. For appropriate tasks (now only reconstruction) the training task saves at the end the input & output tensors corresponding to one image.
  2. model becomes an option and a default architecture exists for all modes (it depends only on the task manager)

@14thibea 14thibea changed the base branch from master to dev July 20, 2021 10:20
@pep8speaks
Copy link

pep8speaks commented Jul 20, 2021

Hello @14thibea! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-05 12:28:39 UTC

@14thibea 14thibea linked an issue Jul 27, 2021 that may be closed by this pull request
@@ -6,47 +6,49 @@

Copy link
Contributor

@nburgos nburgos Jul 28, 2021

Choose a reason for hiding this comment

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

It would be good to explain on the home page what ClinicaDL does in a few words and how it is related with Clinica. This paragraph should also mention in what context ClinicaDL was initially developed as some functions are quite Alzheimer-specific (e.g. restrict or get labels in tsvtools).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered in e0c51a8

@@ -0,0 +1,21 @@
!!! warning "Coming soon"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful to make this page available now? It's for the reconstruction case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is useful to explain the output structure.
I added the purpose of this command at the beginning of the file in 66372d5

- `map.npy` is the numpy array corresponding to the saliency maps and can be loaded with `numpy.load`.

The output tree for the `individual` level is quite similar, except that one occlusion is created
per session. Then the output tree also includes the `participant_id` and `session_id`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong formatting (no list displayed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not change this part as it seems that it disappeared?...

@14thibea 14thibea marked this pull request as ready for review August 2, 2021 12:47
Copy link
Contributor

@mdiazmel mdiazmel left a comment

Choose a reason for hiding this comment

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

Hi @14thibea ,

Thanks for this huge PR!

I have several comments because this contribution mix mainly two things: interface changes of the MAPS class and documentation.

  • My initial though is that some changes would be in conflict with PR#172, so maybe @ravih18 should be aware of the eventual modifications introduced by this PR.
  • New models where uploaded to the CI agents but there are still some problems with the MAPSManager class (see here and example).

Concerning the documentation:

  • I'm not sure of the titles for the sections. I would prefer shorter and self-explanatory titles (in some way what has been done in Clinica documentation). I needed to scroll to the end of the Home section to understand that these titles corresponds to the functions proposed by the software.
  • We need to uniform where and when we write ClinicaDL and clinicadl: The first for the full text and the second one only for the commands.

I added some other comments inside the code.

I propose to merge this PR once the CI problems are solved and once we are agree of the section titles (and in general, about the structure of the documentation). In that way, the PR#172 can integrate modifications and progress in parallel. Other modifications (if necessary) will be done in additional PRs.

@mdiazmel mdiazmel merged commit 66ad25d into aramis-lab:dev Aug 5, 2021
@14thibea 14thibea deleted the refactor_documentation branch August 25, 2021 08:36
mdiazmel added a commit to mdiazmel/clinicadl that referenced this pull request Aug 26, 2021
* Add Contribute section

* Update README to clarify AD-DL separation

* Rename classify in predict

* Fix typo

* Add default network

* Add visualization function

* Fix tests

* Fix interpretation

* Update documentation

* Fix random-search + detach analysis

* Add new sections on contribution

* Fix interpret

* Fix typo

* Refactor interpretation and prediction

* Fix cli + add interpret test

* Train fix

* Train refactoring

* Fix typo

* Minor fixes

* Fix interpret

* Change title in README

* Remove config file at third level

* Answer to the review comments

* Explain AD context

* Fix predict

* Apply suggestions from code review

Co-authored-by: mdiazmel <[email protected]>

* Update docs/Train/Details.md

Co-authored-by: mdiazmel <[email protected]>

* Improve documentation accoding to review comments

* Add docstring to meta-maps

* Execute predict without GPU

* Remove meta-maps test

* Fix Jenkinsfile

Co-authored-by: mdiazmel <[email protected]>
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.

Better describe ClinicaDL train output
4 participants