Skip to content

examples: refine tensor_sum_elements(tensor dump) in examples/benchmark/benchmark-matmult.cpp #7844

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

Closed
wants to merge 6 commits into from

Conversation

zhouwg
Copy link
Contributor

@zhouwg zhouwg commented Jun 10, 2024

Self Reported Review Complexity

  • Review Complexity : Low
  • Review Complexity : Medium
  • Review Complexity : High
  • I have read the contributing guidelines

BTW, can the original author or core maintainer add a

void ggml_tensor_print(const struct ggml_tensor * tensor, const char * name) 

utility function (with all supported data type and dump the accurate value in the tensor as m ( < 8) x n ( < 8) format) in ggml.h&ggml.c, it will might be very useful/helpful for programmers/developers. thanks so much.

@mofosyne mofosyne added the Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level label Jun 12, 2024
@zhouwg zhouwg changed the title examples: refine tensor dump examples: refine tensor dump in examples/benchmark/benchmark-matmult.cpp Jun 13, 2024
@zhouwg zhouwg requested a review from ngxson June 14, 2024 01:51
@zhouwg zhouwg force-pushed the refinetensordump branch 3 times, most recently from 52836a4 to b7a9d40 Compare June 14, 2024 02:54
@zhouwg zhouwg requested a review from ngxson June 14, 2024 15:05
@zhouwg zhouwg changed the title examples: refine tensor dump in examples/benchmark/benchmark-matmult.cpp examples: refine tensor_sum_element(tensor dump) in examples/benchmark/benchmark-matmult.cpp Jun 14, 2024
@zhouwg zhouwg changed the title examples: refine tensor_sum_element(tensor dump) in examples/benchmark/benchmark-matmult.cpp examples: refine tensor_sum_elements(tensor dump) in examples/benchmark/benchmark-matmult.cpp Jun 14, 2024
@zhouwg zhouwg requested a review from ngxson June 14, 2024 15:19
Copy link
Collaborator

@ngxson ngxson left a comment

Choose a reason for hiding this comment

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

LGTM. Let's merge when the CI pass.

Copy link
Member

@slaren slaren left a comment

Choose a reason for hiding this comment

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

What is the goal of these changes when this function is only called with F32 tensors?

@zhouwg
Copy link
Contributor Author

zhouwg commented Jun 14, 2024

What is the goal of these changes when this function is only called with F32 tensors?

programmer/developer might be change data type in this example manually. this PR has no any side-effect.

accordingly, it will helpful for ggml's users(programmers/developers) if there is a tensor dump utility function in ggml.h&ggml.c. this is preparation.of course, this utility function can be provided by you or the original author because you are both expert.

@slaren
Copy link
Member

slaren commented Jun 14, 2024

There is code for printing tensors in the eval-callback example. We could consider moving and possibly extending the functions used to do this in eval-callback to common.cpp so that it is easier to use them during debugging.

@zhouwg
Copy link
Contributor Author

zhouwg commented Jun 14, 2024

your concern is make sense. you can make/made any decision as your consideration because you are one of the owners/core maintainers of this project.

@zhouwg
Copy link
Contributor Author

zhouwg commented Jun 14, 2024

LGTM. Let's merge when the CI pass.

thanks for your help and thanks so much and have a good weekend.

@ngxson
Copy link
Collaborator

ngxson commented Jun 14, 2024

Good point @slaren , I haven’t looked deeper into this file.

@zhouwg I’m a bit doubt about the purpose of this function. Just to be sure: do you need this func to print the tensor? It seems to me that the current func only calculate the sum, but not for printing it out.

@zhouwg
Copy link
Contributor Author

zhouwg commented Jun 15, 2024

Good point @slaren , I haven’t looked deeper into this file.

@zhouwg I’m a bit doubt about the purpose of this function. Just to be sure: do you need this func to print the tensor? It seems to me that the current func only calculate the sum, but not for printing it out.

your are right.this function following the existing tensor_sum_elements but the calculation process can be used for tensor dump,

@ggerganov ggerganov closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Review Complexity : Medium Generally require more time to grok but manageable by beginner to medium expertise level
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants