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

[Doc] [refactor] Improve Taichi kernels and functions definition #1576

Merged
merged 22 commits into from
Aug 11, 2020

Conversation

archibate
Copy link
Collaborator

@archibate archibate added doc Documentation related issues & PRs skip ci labels Jul 24, 2020
Copy link
Contributor

@zhai-xiao zhai-xiao left a comment

Choose a reason for hiding this comment

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

Great job! Some nits here and there, cheers 🥂

docs/hello.rst Outdated Show resolved Hide resolved
docs/hello.rst Outdated

.. warning::

Taichi kernels must be called in the Python-scope. I.e., **nested kernels are not supported**.
Nested functions are allowed. **Recursive functions are not supported for now**.

But nested functions are allowed. **Recursive functions are not supported for now**.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think the but is appropriate here. Could you either remove the but or swap the order of the last 2 sentences (and turn the period into a comma)?

docs/syntax.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
docs/type.rst Outdated Show resolved Hide resolved
docs/type.rst Outdated Show resolved Hide resolved
docs/type.rst Outdated Show resolved Hide resolved
docs/type.rst Outdated
- ``i32 + f32 = f32`` (integer + float = float)
- ``i32 + i64 = i64`` (less-bits + more-bits = lamore-bits)

Basically it will try to choose the least precise type to contain the result value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Umm... I'm not following here. Do you mean 'most'?

docs/type.rst Outdated Show resolved Hide resolved
@archibate archibate requested a review from Rullec July 25, 2020 05:41
@archibate archibate self-assigned this Jul 25, 2020
Co-authored-by: Xiao Zhai <7610945+TroyZhai@users.noreply.github.com>
docs/syntax.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
docs/hello.rst Outdated Show resolved Hide resolved
@archibate archibate requested a review from zhai-xiao July 25, 2020 06:54
docs/syntax.rst Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 25, 2020

Codecov Report

Merging #1576 into master will decrease coverage by 0.03%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1576      +/-   ##
==========================================
- Coverage   44.11%   44.08%   -0.04%     
==========================================
  Files          41       41              
  Lines        5905     5907       +2     
  Branches     1024     1025       +1     
==========================================
- Hits         2605     2604       -1     
- Misses       3151     3153       +2     
- Partials      149      150       +1     
Impacted Files Coverage Δ
python/taichi/lang/kernel.py 68.87% <25.00%> (-0.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18cfb8...4b79e5b. Read the comment docs.

docs/type.rst Outdated Show resolved Hide resolved
docs/hello.rst Outdated Show resolved Hide resolved
docs/hello.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@zhai-xiao zhai-xiao left a comment

Choose a reason for hiding this comment

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

Hi @archibate , great job! Just one minor thing, I remembered that there was a place that you mentioned the type will be automatically casted to the most precise type (like float + double -> double) in binary operations, but I think you used 'least precise' or something like that. Could you explain the reason for that?

docs/hello.rst Outdated Show resolved Hide resolved
docs/hello.rst Outdated Show resolved Hide resolved
@archibate archibate changed the title [Doc] [refactor] Improve syntax.rst, type.rst [Doc] [refactor] Improve hello.rst, syntax.rst readability Jul 31, 2020
@archibate archibate requested a review from zhai-xiao July 31, 2020 07:41
@archibate archibate requested a review from isdanni August 10, 2020 03:40
@archibate archibate changed the title [Doc] [refactor] Improve hello.rst, syntax.rst readability [Doc] [refactor] Improve Taichi kernels and functions definition Aug 10, 2020
Copy link
Contributor

@isdanni isdanni left a comment

Choose a reason for hiding this comment

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

Thanks! Only a few nits ⚡

docs/hello.rst Outdated Show resolved Hide resolved
docs/hello.rst Outdated Show resolved Hide resolved
docs/syntax.rst Outdated Show resolved Hide resolved
Co-authored-by: Danni <38550500+isdanni@users.noreply.github.com>
@archibate archibate requested a review from isdanni August 10, 2020 15:28
Copy link
Contributor

@isdanni isdanni left a comment

Choose a reason for hiding this comment

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

LGTM! thanks!

@archibate archibate added the LGTM label Aug 11, 2020
@archibate archibate merged commit 5c67f78 into taichi-dev:master Aug 11, 2020
@yuanming-hu yuanming-hu mentioned this pull request Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation related issues & PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants