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

Add docs for histogram #503

Merged
merged 10 commits into from
Mar 24, 2021
Merged

Conversation

codejaeger
Copy link
Contributor

@codejaeger codejaeger commented Jun 26, 2020

Description

Add documentation for the histogram functionality provided in PR #499 .

References

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass - RIP Travis CI, replaced with GitHub Actions
  • Review and approve

@codejaeger codejaeger marked this pull request as draft June 26, 2020 21:25
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

It looks very good!
Quick review of the docs structure:

  1. No need for core/ subdirectory, just move the files one level up, and remove the directory. The name core is an informal one, that, there is no physical structure named core.

  2. The `extension/' directory does make sense, please keep it.

  3. Please, remove _a_ infix and _it suffix from file names. Simple names like create.rst, fill.rst, subhistogram.rst are clear enough.

 - Made changes suggested in first review

 - Add docs for relevant files
@codejaeger codejaeger requested a review from mloskot June 29, 2020 16:03
@codejaeger codejaeger marked this pull request as ready for review June 29, 2020 16:03
@mloskot mloskot added cat/documentation google-summer-of-code All items related to GSoC activities labels Jun 30, 2020
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Nitpick: The file name cumulativehistogram.rst should read cumulative.rst. It already lives in histogram/ directory.

@codejaeger codejaeger requested a review from mloskot July 1, 2020 08:27
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

@codejaeger Well done! It's great to see GSoC features covered with documentation :-) I'm happy to approve this PR. No objections from you @lpranam ?


I have offered a few comments, but those are not show-stoppers.
Technical writing is not a trivial activity. We can always improve and refine documentation in future.


I'd suggest we merge it after we merge PR #499.
Next time you may prefer to stick docs in the same PR as implementation and tests.

doc/histogram/create.rst Outdated Show resolved Hide resolved
doc/histogram/create.rst Outdated Show resolved Hide resolved
doc/histogram/cumulative.rst Outdated Show resolved Hide resolved
doc/histogram/extend.rst Outdated Show resolved Hide resolved
doc/histogram/extend.rst Outdated Show resolved Hide resolved
doc/histogram/extend.rst Outdated Show resolved Hide resolved
doc/histogram/extend.rst Outdated Show resolved Hide resolved
doc/histogram/fill.rst Outdated Show resolved Hide resolved
Comment on lines 62 to 78
**Grayscale Image**

.. figure:: barbara.png
:width: 600px
:align: center
:height: 300px
:alt: Could not load image.
:figclass: align-center

**RGB**

.. figure:: church.png
:width: 600px
:align: center
:height: 250px
:alt: Could not load image.
:figclass: align-center
Copy link
Member

Choose a reason for hiding this comment

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

I do not like the idea of storing 7MB files in GIL's repo.
Unfortunately, I can not offer an ideal alternative yet.

I'd suggest a temporary solution:

  1. Remove the images from this PR
  2. Paste the images as comments to this PR
  3. Replace filename with URLs from GitHub
.. figure:: https://user-images.githubusercontent.com/12345/12811123-3877a980-8375-11ea-9960-1b88a3f9f87b.png
...

The thing is, once we start adding big binary files to Git repo, then even if we find an optimal solution for hosting images for docs, we will not be able to get rid of them easily, from Git history.

I have (slowly) started collecting externally hosted test images in dedicated repo https://github.com/boost-gil/test-images, so I perhaps we could have examples/ folder there and link from docs to there.

What do you think @lpranam ?

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, in docs we don't have to use the full-size images. Docs just need to demonstrate what our algorithms can do and do not need to show the exact accurate images, we can use highly compressed and small resolution images in docs. Otherwise what @mloskot suggested the great. I have no other objections regarding this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codejaeger Well done! It's great to see GSoC features covered with documentation :-) I'm happy to approve this PR.

Thanks a lot @mloskot : )

I'd suggest we merge it after we merge PR #499.
Next time you may prefer to stick docs in the same PR as implementation and tests.

Yes sure. Also this PR contains docs for the algorithm histogram equalization. I haven't pushed it yet since the histogram class is a dependency for it. Should I remove the docs for the algorithm?

Copy link
Contributor Author

@codejaeger codejaeger Jul 31, 2020

Choose a reason for hiding this comment

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

I have compressed the images, they now occupy about 34 KB and 24 KB. I can also do what @mloskot suggested if needed.

Copy link
Member

Choose a reason for hiding this comment

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

though it is compressed for doc would be great to add real one on that repo so we can have those images for later.

Copy link
Member

Choose a reason for hiding this comment

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

@codejaeger If you want to try the idea outlined earlier, you should have now the write access to https://github.com/boost-gil/test-images Feel free to create doc/ directory (and any necessary subfolders) and put your images in there.

Copy link
Member

Choose a reason for hiding this comment

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

@codejaeger Could you add full size/resolution images to the test-images repo as @lpranam suggested?

Copy link
Member

Choose a reason for hiding this comment

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

@codejaeger Ping ☝️

Copy link
Contributor Author

@codejaeger codejaeger Mar 24, 2021

Choose a reason for hiding this comment

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

Yep doing it. Thanks for the reminder, sorry I had totally forgotten about it.

@mloskot
Copy link
Member

mloskot commented Jan 24, 2021

@codejaeger I've just git merge-updated your docs_for_histogram branch from the latest develop

@mloskot mloskot added this to the Boost 1.77+ milestone Mar 24, 2021
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #503 (b09a0c2) into develop (0c0fe1a) will increase coverage by 0.78%.
The diff coverage is 98.35%.

@@             Coverage Diff             @@
##           develop     #503      +/-   ##
===========================================
+ Coverage    77.81%   78.59%   +0.78%     
===========================================
  Files          110      117       +7     
  Lines         4367     5003     +636     
===========================================
+ Hits          3398     3932     +534     
- Misses         969     1071     +102     

@mloskot mloskot merged commit dc9ba74 into boostorg:develop Mar 24, 2021
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
* Add docs for histogram

* Add docs and make changes:

 - Made changes suggested in first review

 - Add docs for relevant files

* Rename docs file and correct typos

* Change doc for cumulative histogram

* Add docs for histogram equalization

* Make changes suggested in review

* Add docs

* Remove docs for algorithms

* Move images to test_images

Co-authored-by: Mateusz Łoskot <mateusz@loskot.net>
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 21, 2021
* Add docs for histogram

* Add docs and make changes:

 - Made changes suggested in first review

 - Add docs for relevant files

* Rename docs file and correct typos

* Change doc for cumulative histogram

* Add docs for histogram equalization

* Make changes suggested in review

* Add docs

* Remove docs for algorithms

* Move images to test_images

Co-authored-by: Mateusz Łoskot <mateusz@loskot.net>
meshtag pushed a commit to meshtag/gil that referenced this pull request Apr 22, 2021
* Add docs for histogram

* Add docs and make changes:

 - Made changes suggested in first review

 - Add docs for relevant files

* Rename docs file and correct typos

* Change doc for cumulative histogram

* Add docs for histogram equalization

* Make changes suggested in review

* Add docs

* Remove docs for algorithms

* Move images to test_images

Co-authored-by: Mateusz Łoskot <mateusz@loskot.net>
@mloskot mloskot mentioned this pull request May 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/documentation google-summer-of-code All items related to GSoC activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants