Skip to content

Conversation

@jeffzhou2000
Copy link

@jeffzhou2000 jeffzhou2000 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
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?

@jeffzhou2000
Copy link
Author

jeffzhou2000 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.

@jeffzhou2000
Copy link
Author

jeffzhou2000 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.

@jeffzhou2000
Copy link
Author

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.

@jeffzhou2000
Copy link
Author

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