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

Add option to find_upstream to keep nodes visited even if not found #83

Merged

Conversation

iksnagreb
Copy link
Contributor

Sometimes you want to seek upstream until you either find a particular node or terminate at an input or initializer. This is almost handled by find_upstream, except for it returning the empty list in case the search terminates. This is reasonable default behavior. By adding a flag, handling the second use case is almost free. The option is disabled by default to not change the existing behavior.

By the way: Is there a particular reason to return the empty list if there is no producer and to return None if there is a producer without inputs? It is probably of interest to distinguish these tow cases, but was this a deliberate choice utilized somewhere? This behavior is not changed by this PR.

@iksnagreb
Copy link
Contributor Author

iksnagreb commented Nov 14, 2023

Hm, some tests fail due to ValueError: Could not infer attribute explicit_paddings type from empty iterator somewhere within onnx. Nothing I have touched, seems to be due to a recent onnx update while the onnx version here is not pinned exactly, it uses the >= specifier.

@maltanar
Copy link
Collaborator

Hi Christoph,
The ValueError: Could not infer attribute explicit_paddings type from empty iterator problems were due to an unrelated package which is now fixed. Adding changes from latest main to re-trigger the CI now.
Regarding the None vs [] return behavior, I believe this is due to some legacy code in FINN that is using one variant or another - @auphelia might have a better idea here. I'm not aware if this is being used to distinguish something useful.

@maltanar
Copy link
Collaborator

CI runs now looking good. Since the new parameter defaults to the old behavior when not specified I think this should be a safe inclusion into the codebase. Thanks for the PR!

@maltanar maltanar merged commit fe4aa37 into fastmachinelearning:main Feb 23, 2024
5 checks passed
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.

2 participants