ONE-vscode
ONE-vscode copied to clipboard
Samsung
[MPQEditor] Fix showing `Model Properties` on input/output nodes
What?
Let's fix showing Model Properties on input/output nodes.
Why?
See original discussion in https://github.com/Samsung/ONE-vscode/pull/1539#discussion_r1179858673
There are 3 options available as a summary of https://github.com/Samsung/ONE-vscode/pull/1539#discussion_r1179858673, https://github.com/Samsung/ONE-vscode/pull/1539#discussion_r1179928767, https://github.com/Samsung/ONE-vscode/pull/1539#discussion_r1179942615:
- We can hide input/output nodes
- We can disable clicking input/output nodes
- Show properties for internal nodes as well as for input/output nodes.
@dayo09 @seanshpark @jinevening
Please correct me if i'm wrong (or may be there are other suggestions):
IMHO it is very convenient to invoke the main action of the MPQEditor (which is selection) by clicking an item, but to show Model Properties or internal node properties by invoking ctrl+enter or properties command from item context menu.
So as a first try it's possible:
- to block 'click' on 'input/output' node
- provide properties by 'Alt+Enter'
- provide properties from 'context menu'
- may be to make properties more relevant to quantization (ranges of channels weights or activations or their distribution depicted graphically)
I don't understand the problem discussed in #1539. please explain what the problem is.
@seanshpark By clicking a node, MPQEditor is expected to select the node for editing quantization settings, but when you click an IO nodes, it only opens the model properties view. It confuses users because it shows IO properties while remaining selection on other nodes.
@stamalakhov
IMO, I'd like to see properties for all the nodes. It would be nice if we can open properties view for all the nodes while enabling valid selection on the only valid nodes. If it's too hard or far to go, Id' like (1) as an alternative.
@stamalakhov IMO, I'd like to see
propertiesfor all the nodes. It would be nice if we can openpropertiesview for all the nodes while enabling valid selection on the only valid nodes. If it's too hard or far to go, Id' like (1) as an alternative.
I agree.
@dayo09
Do you mean that adding nodes for editing their quantization properties and showing properties should be invoked just by clicking nodes?
If yes:
then we would need to provide another mode for viewMode, because for now it uses selector mode, used in PartitionEditor.
If no (different actions: click for selection and alt+enter for properties):
then it will be ok to fix (1) and then to add showing properties by Alt+Enter later.
@stamalakhov
In my personal opinion, yes. But it may vary by person. Maybe we can vote or you can decide by yourself by this moment, depending on the implementation load. 😃
@dayo09 @seanshpark @jinevening please see https://github.com/Samsung/ONE-vscode/issues/1540#issuecomment-1530872999 and https://github.com/Samsung/ONE-vscode/issues/1540#issuecomment-1531020142:
Would it be better in MPQEditor to simultaneously invoke Show Properties and Select nodes for editing quantization parameters by clicking nodes in CircleGraph?
Would it be better in MPQEditor to simultaneously invoke Show Properties and Select nodes for editing quantization parameters by clicking nodes in CircleGraph?
I don't know without using the GUI, and I can't test your code as of now. My opinion is go as is; don't care about the property in I/O nodes. Land all your codes. We can fix your design later. Why? frankly speaking, I am not satisfied with your GUI.
Why? frankly speaking, I am not satisfied with your GUI.
@seanshpark May i ask you a question? What's wrong with GUI?
May i ask you a question? What's wrong with GUI?
I didn't wrote what is wrong. I am not satisfied your design.
Q) does the current main branch work like this issue? or is this issue about your draft?
Q) does the current
mainbranch work like this issue? or is this issue about your draft?
Yes. The 'main' branch works like described above, showing properties for input/output nodes.