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

Improve convolution batch support documentation #2639

Merged
merged 2 commits into from
Sep 20, 2019

Conversation

9prady9
Copy link
Member

@9prady9 9prady9 commented Sep 7, 2019

Fixes #2625

Merge arrayfire/assets#11 before merging this PR

@9prady9 9prady9 added this to the v3.7.0 milestone Sep 7, 2019
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/doxygen.mk Show resolved Hide resolved
@mark-poscablo
Copy link
Contributor

mark-poscablo commented Sep 9, 2019

Posting this image here for reference on how the equations for convolve2 were rendered (at least on my macbook):
image

@umar456
Copy link
Member

umar456 commented Sep 9, 2019

Hmm. That doesn't look very readable.

Copy link
Contributor

@mark-poscablo mark-poscablo left a comment

Choose a reason for hiding this comment

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

I have some feedback generally about grammar and how the rendered docs look.

Also, can we add the separators between each convolution group in the dox file? So it's easier to distinguish which section is which in the dox (by separators, I mean the usual comment line, like below)
//=====================================================================

src/api/c/convolve.cpp Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
@9prady9
Copy link
Member Author

9prady9 commented Sep 10, 2019

Hmm. That doesn't look very readable.

I think the equations are:

  1. very much readable
  2. easy to understand provided the user knows set notation (no fancy notation, basic set notation).
  3. last but not least, easy to express the kind of batch operation taking place.

P.S. Since AF is a math library we can assume safely that basic set notation is known to users.

docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
include/af/ml.h Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Outdated Show resolved Hide resolved
docs/details/signal.dox Show resolved Hide resolved
@9prady9
Copy link
Member Author

9prady9 commented Sep 11, 2019

Updated documentation, I think it is more clear and precise (without ambiguity) than before now.
Screenshot from 2019-09-11 10-52-24

Also includes the following changes
* Fix reference warnings in ml header
* Improve conv docs in general
Copy link
Contributor

@mark-poscablo mark-poscablo left a comment

Choose a reason for hiding this comment

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

Approving. Thanks for all the work @9prady9, appreciate it.

@9prady9 9prady9 merged commit 929a641 into arrayfire:master Sep 20, 2019
@9prady9 9prady9 deleted the conv_docs_fix branch September 20, 2019 16:12
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.

How to implement tensorflow/pytorch like convolution?
3 participants