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

MegEngine: fix some bugs #1022

Closed
wants to merge 3 commits into from
Closed

MegEngine: fix some bugs #1022

wants to merge 3 commits into from

Conversation

Ysllllll
Copy link

fix some bugs of megengine C++ model (.mge) visualization:

  1. show the shape of the middle tensor;
  2. fix scope matching model identifier (mgv2) due to possible leading information;

please help review, thanks~

source/megengine.js Outdated Show resolved Hide resolved
source/megengine.js Outdated Show resolved Hide resolved
source/megengine.js Outdated Show resolved Hide resolved
[ 'quantizeds1', 'int8' ], [ 'quantizeds4', 'int8' ], [ 'quantizeds8', 'int8' ], [ 'intb1', 'int8' ], [ 'intb2', 'int8' ], [ 'intb4', 'int8' ], [ 'qint8', 'int8' ],
[ 'quantizeds16', 'int16' ],
[ 'quantizeds32', 'int32' ]
[ 'bool', 'boolean' ], [ 'byte', 'uint8' ], [ 'quantizeds4asymm', 'uint8' ], [ 'quantizeds8asymm', 'uint8' ], [ 'uintb4', 'uint8' ], [ 'quantizeds1', 'int8' ], [ 'quantizeds4', 'int8' ], [ 'quantizeds8', 'int8' ], [ 'intb1', 'int8' ], [ 'intb2', 'int8' ], [ 'intb4', 'int8' ], [ 'qint8', 'int8' ], [ 'quantizeds16', 'int16' ], [ 'quantizeds32', 'int32' ], [ 'TypePlaceHolder', 'boolean' ]
Copy link
Owner

Choose a reason for hiding this comment

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

Each pair on new line to make diff easier in the future?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, done.

Copy link
Owner

Choose a reason for hiding this comment

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

Why is ? mapped to boolean via [ 'bool', 'boolean' ], [ '?', 'boolean' ]? Also this._dataTypeName and this._dataType should be identical so toString() uses the same name as type.dataType?

Copy link
Author

Choose a reason for hiding this comment

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

After testing, ['?', 'boolean'] here can be deleted.
The this._dataType here is mainly used to dynamically read the numerical data of the megengine.Tensor during visualization. It seems that the reading of the numerical data of the *.Tensor is no longer customized by the .js files of various frameworks? Therefore, this._dataTypeName is used to display the data type of mge.

Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to generalize this? For example, can these types be added and supported for other formats as well by the shared rendering code?

Copy link
Author

Choose a reason for hiding this comment

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

The mapping here only involves some special data types (such as quantizeds4asymm), but there is no mapping for common data types (such as float32). For mge models in other formats (such as .tm format), the special data types they support are also included in this correspondence. So don't generalize? I wonder if I understand what you mean correctly?

Copy link
Owner

Choose a reason for hiding this comment

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

Instead of mapping quantizeds4asymm to uint8, is quantizeds4asymm a common data type that is supported by other frameworks? What would be the right name to use for all frameworks? Should dialog.Tensor support these data types? For example, there is `int4' in other frameworks, if it is not encoded in a standard way maybe it should report the standard name and not return data for rendering?

Copy link
Author

@Ysllllll Ysllllll Jan 9, 2023

Choose a reason for hiding this comment

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

Data type quantizeds4asymm is defined by us for quantifying data types, so it is not a common data type supported by other frameworks. Its corresponding name in dialog.Tensor should be quint8, which is also the name used by other frameworks. It has been fixed in the code.

tools/megengine Outdated Show resolved Hide resolved
@Ysllllll
Copy link
Author

Hi @lutzroeder, can this mr be merged?

@lutzroeder
Copy link
Owner

lutzroeder commented Jan 27, 2023

@Ysllllll can you rebase to TIP. The mgbtest0 and ./tools/megengine changes should be no longer needed. If not, please share a small repro. The data type support is still an open issue. For now, use the specific name if there is no general name to get unblocked. This will result in the uint8 not getting rendered which can be addressed in a later change.

@Ysllllll
Copy link
Author

Ysllllll commented Jan 29, 2023

OK. Yes,

  • for ./tools/megengine, its changes is no longer needed.
  • for mgbtest0, need to modify a little bit, that is, remove the if statement ( if (size === (stream.length - position - 4))).
    This is mainly because the current model file also contains data, and the size of the record is only the size of the model, without adding the size of the data.
    Therefore, the correct expression should be size === (stream.length - position - 4) - length(inputdata), but since the data size is unknown, the if statement needs to be removed at this time.
    Here is an example:
    add_with_data.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants