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

Wrap input names in PyLayer #683

Merged
merged 4 commits into from Jul 15, 2022
Merged

Conversation

zimka
Copy link
Contributor

@zimka zimka commented Jul 13, 2022

Currently DNN graph structure is inaccessible from NeoML/Python - you can get the list of layers, but you can not read how these layers are connected with each other.
With input names getter it is possible to traverse the whole graph starting from sinks and retrieve it's structure.

It could be used in theory to plot the model, similar to torchviz or maybe even in a plugin for netron.

Also fixes incorrect assertTrue which are supposed to be assertEqual

Signed-off-by: Boris Zimka <boris.zimka@abbyy.com>
@FedyuninV
Copy link
Contributor

The idea is really good, but I think it's better to provide list of tuples (input_layer_name, output_index). Current solution doesn't provide enough info if model has layers with multiple outputs (e.g. SplitSomeDimension).

Signed-off-by: Boris Zimka <boris.zimka@abbyy.com>
@zimka
Copy link
Contributor Author

zimka commented Jul 15, 2022

I agree that input_output_index is necessary to retreive precise graph structure, but in my opinion the list of tuples (input_layer_name, output_index) is not the best way to handle the problem.

  1. Output index of input is needed only in some specific cases, while in most cases there is either no order at all (single output) or the order does not matter (eltwise sum/prod)
  2. "Output Index of Input with Index" is quite clumsy concept, which is not obvious from the first glance. It could be better to wrap CDnnLayerLink, which has more intuitive meaning, but this is out of this PR scope

I have added one more method get_input_output_index and tests for it, please check if you are fine with it

@FedyuninV
Copy link
Contributor

FedyuninV commented Jul 15, 2022

  1. Agree. My idea now is to add both input_names and inputs.
  2. Agree, but I think we can avoid such awkward names like get_input_output_idx.

Here is the idea:

@property
def inputs(self):
   ''' Tuple in which i'th element contains information about the layer connected to the i'th input of `self` (name, output number)
   '''
   return tuple( ... )

@property
def input_names(self):
   ''' Tuple in which i'th element contains the name of the layer, connected to the  i'th inputs of `self`
   '''
   return tuple( layer_name for layer_name, _ in self.inputs )

The things I don't like in current implementation are Why we have input_names as property, but we have to call method with possibility of wrong idx being passed for getting info about output index? Why these methods are so different?

Signed-off-by: Boris Zimka <boris.zimka@abbyy.com>
@zimka
Copy link
Contributor Author

zimka commented Jul 15, 2022

Yep, these questions sound reasonable.
I have replaced .get_input_output_idx in PyLayer interface with input_links property, it returns tuple of tuples. I have kept the internal method, because it actually mimics CBaseLayer interface.

I think .inputs would be unclear, because it could be layer instances for example, IMO input_links describes it more precisely.
Hope one day there will be pybinded CDnnLayerLinks instead of tuples

@FedyuninV FedyuninV merged commit 2ad8de3 into neoml-lib:master Jul 15, 2022
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