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

feat: feed shape info to initializer #78

Closed
wants to merge 1 commit into from

Conversation

zimond
Copy link
Contributor

@zimond zimond commented Mar 23, 2022

fixes #66

Turns out that initializer inited info proto will not include the shape info. This PR forces callers to provide shape information to ensure that any shaders using the info would not crash

@zimond zimond changed the title feat: feed info shape to initializer feat: feed shape info to initializer Mar 23, 2022
@pixelspark pixelspark self-requested a review March 23, 2022 09:30
@pixelspark
Copy link
Collaborator

Thanks for the PR @zimond!

The only issue I have with this is that we are now repeating dimensions twice for an initializer (first in the 'infos' list and next in the 'initializers' list). This could lead to errors if the dimensions passed to tensor(..) are different from the dimensions passed to initializer(...). From e.g. the test for BatchNormalization:

let bn_model = model(graph(
        vec![tensor("X", &shape)],
        vec![tensor("Y", &shape)],
        vec![
            tensor("scale", &[channels as i64]),  // First here...
            tensor("B", &[channels as i64]),
            tensor("input_mean", &[channels as i64]),
            tensor("input_var", &[channels as i64]),
        ],
        vec![
            initializer("scale", scale, vec![channels as i64]), // Then here again
            initializer("B", b, vec![channels as i64]),
            initializer("input_mean", mean, vec![channels as i64]),
            initializer("input_var", var, vec![channels as i64]),
        ],
        // ...
    ));

I would suggest to auto-generate the 'infos' for each initializer, so you can omit the 'infos' for initializers. This should be quite an easy fix (and save a few lines in the test code).

Copy link
Collaborator

@pixelspark pixelspark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@zimond
Copy link
Contributor Author

zimond commented Mar 24, 2022

How could you auto-generate the infos as initializer is just accepting 1-d arrays?

@pixelspark
Copy link
Collaborator

How could you auto-generate the infos as initializer is just accepting 1-d arrays?

The infos just takes the name and dimensions, which you are both also supplying to initializer(..). My suggestion is to have graph(..) automatically generate the infos for initializers, so you don’t have to call info(..) for those (and potentially make the mistake of calling info and initializer with different dimensions for the same tensor).

@pixelspark
Copy link
Collaborator

pixelspark commented Mar 26, 2022

See #80

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.

graph info shape is empty
2 participants