-
Notifications
You must be signed in to change notification settings - Fork 51
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
[MPQEditor] Create or edit mixed precision quantization json file. #1505
base: main
Are you sure you want to change the base?
Conversation
3702a75
to
ea65d9e
Compare
media/CircleGraph/index.js
Outdated
@@ -52,6 +52,7 @@ const viewMode = { | |||
viewer: 0, // default circle viewer | |||
selector: 1, // circle partition editor node selector | |||
visq: 2, // quantization error viewer | |||
visqselr: 3, //quantization error viewer whicj is able to select nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo whicj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this mode added?
what is visqselr
? this word is hard to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this mode added?
new mode is intended to provide .visq colormap, as well as selector
capabilities to provide a user handy selection of nodes.
what is
visqselr
? this word is hard to read.
Yes. The name is hard to read, i'll gladly change it with any other more fortunate name.
media/CircleGraph/index.js
Outdated
} else if (message.mode === "visq") { | ||
this._mode = viewMode.visq; | ||
} else if (message.mode === "visqselr") { | ||
this._mode = viewMode.visqselr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only put this line as there is no other change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only put this line as there is no other change
Sorry. I don't quite get it. Current implementation assumes that we can change html
_mode
to another mode, so these lines are intended to provide full enumeration of viewMode
, but for now only visqselr
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but for now only visqselr is enough.
this is the point. check only for visqselr
. setting to other modes are never called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh. Thank you. Ok.
Q) How visq info is created? Is it assumed to be generated before? If so, it would be better to give instruction on how to make visq info (e.g., making a small question mark icon near the "Load visq info" button). BTW, is it possible to create visq info in ONE-vscode? (optional) I think it is better to show the model graph by default when the tab is first created. I guess most users will want to select layers in the graph with visq info. |
Yes, for this draft it was generated before.
ok.
I'll investigate it.
Ok. I'll be back with reworked draft. |
b87a618
to
a9ebb2d
Compare
I've tried to introduce visq data computation into MPQEditor and it occured to be heavy: IMHO for |
a9ebb2d
to
36471ce
Compare
@jinevening @seanshpark |
Thanks for video attach at #1505 (comment) :) Q1) what happens if wrong json (not visq info content) is selected in visq info ?
|
src/MPQEditor/MPQCircleSelector.ts
Outdated
} | ||
|
||
public onLoadVisqInfo(visqPath: string) { | ||
const fileData = fs.readFileSync(visqPath, { encoding: "utf8", flag: "r" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, it's rather safer to use vscode.workspace.fs.readFile
instead of fs.readFileSync. Related issue: #746
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Thank you. I didn't know that.
src/MPQEditor/MPQCircleSelector.ts
Outdated
if (this._panel.visible) { | ||
// NOTE if we call this.update(), it'll reload the model which may take time. | ||
// TODO call conditional this.update() when necessary. | ||
// this.update(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q. Is it fine with this.update()
being commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... Sorry. I'll remove that.
src/MPQEditor/MPQCircleSelector.ts
Outdated
// let parsedPath = path.parse(args.docPath); | ||
// let fileBase = path.join(parsedPath.dir, parsedPath.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be removed, if it's not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh.. Sorry. These lines will be removed.
docs/MPQEditor.md
Outdated
5) all layers from 'layers' section in json are shown in grid-table, so that their options for quantization can be edited using their respective dropdowns | ||
6) to make 'model-graph-view' more informative the user may use 'visq' information, it shows which layers are more sensitive to quantization than others | ||
|
||
<img src="./images/MPQ_UI.png"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refine this image. do not use red color for text. use smaller size for explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add "Default Parameters" in GUI not in this image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh... Sorry. I'll fix it.
This commit allows to create or edit mixed precision quantization json file.
To use the tool the following actions are suggested:
Related: #1491
ONE-vscode-DCO-1.0-Signed-off-by: s.malakhov s.malakhov@partner.samsung.com