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.

Sign up for GitHub

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

Jump to bottom

[Doc] Add docstring and documents for sparse matrix #3119

Merged
merged 22 commits into from
Oct 11, 2021
Merged

[Doc] Add docstring and documents for sparse matrix #3119

merged 22 commits into from
Oct 11, 2021

Conversation

Hanke98
Copy link
Collaborator

@Hanke98 Hanke98 commented Oct 8, 2021

Related issue = #2906

@netlify
Copy link

netlify bot commented Oct 8, 2021 •
edited
Loading

❌ Deploy Preview for jovial-fermat-aa59dc failed.

🔨 Explore the source changes: 38aee38

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/61641e2fd68b560008397c43

@k-ye k-ye marked this pull request as ready for review October 8, 2021 16:20
@Hanke98 Hanke98 marked this pull request as draft October 8, 2021 23:28
@Hanke98
Copy link
Collaborator Author

Hanke98 commented Oct 9, 2021

Sorry, this pr is still working in progress, more docs about the sparse matrix will come in the following commits soon.

FantasyVR
FantasyVR reviewed Oct 9, 2021
View reviewed changes
Copy link
Collaborator

@FantasyVR FantasyVR left a comment

Choose a reason for hiding this comment

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

Thank you very much @Hanke98!

Just a few nits.

BTW, could you add more docstring for sparse matrix functions?

python/taichi/lang/sparse_matrix.py Outdated Show resolved Hide resolved
@Hanke98
Copy link
Collaborator Author

Hanke98 commented Oct 10, 2021

Thanks!

BTW, could you add more docstring for sparse matrix functions?

Sure, I am working on that :)

k-ye
k-ye reviewed Oct 11, 2021
View reviewed changes
python/taichi/lang/sparse_solver.py Outdated Show resolved Hide resolved
python/taichi/lang/sparse_solver.py Outdated Show resolved Hide resolved
@Hanke98 Hanke98 marked this pull request as ready for review October 11, 2021 01:55
@Hanke98 Hanke98 requested review from k-ye and ltt1598 October 11, 2021 02:01
FantasyVR
FantasyVR reviewed Oct 11, 2021
View reviewed changes
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
python/taichi/lang/sparse_matrix.py Outdated Show resolved Hide resolved
python/taichi/lang/sparse_matrix.py Outdated Show resolved Hide resolved
python/taichi/lang/sparse_matrix.py Outdated Show resolved Hide resolved
python/taichi/lang/sparse_solver.py Outdated Show resolved Hide resolved
python/taichi/lang/sparse_solver.py Outdated Show resolved Hide resolved
FantasyVR
FantasyVR approved these changes Oct 11, 2021
View reviewed changes
Copy link
Collaborator

@FantasyVR FantasyVR left a comment

Choose a reason for hiding this comment

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

LGTM!

docs/lang/articles/advanced/sparse_matrix.md Show resolved Hide resolved
Hanke98 and others added 2 commits October 11, 2021 15:35
@Hanke98
add headings
e2ebb5a
@Hanke98 @FantasyVR
Apply suggestions from code review
2eddf32 
Co-authored-by: FantasyVR <6712304+FantasyVR@users.noreply.github.com>
@Hanke98
Copy link
Collaborator Author

Hanke98 commented Oct 11, 2021

/format

FantasyVR
FantasyVR approved these changes Oct 11, 2021
View reviewed changes
Copy link
Collaborator

@FantasyVR FantasyVR left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry. I find minor nits.

After the modification, I think we could merge this PR now.

python/taichi/linalg/sparse_solver.py Outdated Show resolved Hide resolved
python/taichi/linalg/sparse_solver.py Outdated Show resolved Hide resolved
@Hanke98 @FantasyVR
Apply suggestions from code review
819d4d7 
Co-authored-by: FantasyVR <6712304+FantasyVR@users.noreply.github.com>
FantasyVR
FantasyVR approved these changes Oct 11, 2021
View reviewed changes
Copy link
Collaborator

@FantasyVR FantasyVR left a comment

Choose a reason for hiding this comment

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

I think we could merge this PR. Because the netlify build error has nothing to do with this pr.

k-ye
k-ye approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM!

docs/lang/articles/advanced/sparse_matrix.md Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
k-ye
k-ye approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM!

docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Outdated Show resolved Hide resolved
docs/lang/articles/advanced/sparse_matrix.md Show resolved Hide resolved
@k-ye k-ye merged commit c3478c7 into taichi-dev:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FantasyVR FantasyVR FantasyVR approved these changes

@k-ye k-ye k-ye approved these changes

@ltt1598 ltt1598 Awaiting requested review from ltt1598

@strongoier strongoier Awaiting requested review from strongoier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Hanke98 @ltt1598 @FantasyVR @k-ye @taichi-gardener