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

[MPQEditor] Fix showing Model Properties on input/output nodes #1540

Open
stamalakhov opened this issue Apr 28, 2023 · 12 comments
Open

[MPQEditor] Fix showing Model Properties on input/output nodes #1540

stamalakhov opened this issue Apr 28, 2023 · 12 comments

Comments

@stamalakhov
Copy link
Contributor

What?

Let's fix showing Model Properties on input/output nodes.

Why?

See original discussion in #1539 (comment)

@stamalakhov stamalakhov self-assigned this Apr 28, 2023
@stamalakhov
Copy link
Contributor Author

stamalakhov commented Apr 30, 2023

There are 3 options available as a summary of #1539 (comment), #1539 (comment), #1539 (comment):

  1. We can hide input/output nodes
  2. We can disable clicking input/output nodes
  3. 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:

  1. to block 'click' on 'input/output' node
  2. provide properties by 'Alt+Enter'
  3. provide properties from 'context menu'
  4. may be to make properties more relevant to quantization (ranges of channels weights or activations or their distribution depicted graphically)

@seanshpark
Copy link
Contributor

I don't understand the problem discussed in #1539. please explain what the problem is.

@dayo09
Copy link
Contributor

dayo09 commented May 1, 2023

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

@dayo09
Copy link
Contributor

dayo09 commented May 1, 2023

@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
Copy link
Contributor Author

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

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.

@dayo09
Copy link
Contributor

dayo09 commented May 2, 2023

@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. 😃

@stamalakhov
Copy link
Contributor Author

@dayo09 @seanshpark @jinevening
please see #1540 (comment) and #1540 (comment):

Would it be better in MPQEditor to simultaneously invoke Show Properties and Select nodes for editing quantization parameters by clicking nodes in CircleGraph?

@stamalakhov
Copy link
Contributor Author

IMHO for properties another command should be issued like Alt+Enter.

@seanshpark
Copy link
Contributor

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.

@stamalakhov
Copy link
Contributor Author

Why? frankly speaking, I am not satisfied with your GUI.

@seanshpark
May i ask you a question? What's wrong with GUI?

@seanshpark
Copy link
Contributor

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?

@stamalakhov
Copy link
Contributor Author

Q) does the current main branch 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.

@stamalakhov stamalakhov removed their assignment May 26, 2023
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

No branches or pull requests

3 participants