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

Fix for multiple inputs that may get out of order #937

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

jmduarte
Copy link
Member

@jmduarte jmduarte commented Dec 7, 2023

Fix for multiple inputs that may be in a different order than the order in which the model uses them. Should I add a specific test for this?

  • Bug fix (non-breaking change that fixes an issue)

Tests

This breaks in the current main

import hls4ml
from tensorflow.keras.models import Model
from tensorflow.keras.layers import Dense, Input, Add

input_1 = Input(shape=(10), name='input_1')
input_2 = Input(shape=(10), name='input_2')
x = Dense(10, name='dense_1')(input_2)
outputs = Add(name='add_1')([x, input_1])
model = Model(inputs=[input_1, input_2], outputs=outputs, name='model_1')
model.summary()

config = hls4ml.utils.config_from_keras_model(model, granularity='model')
hls_model = hls4ml.converters.convert_from_keras_model(model,
                                                       hls_config=config,
                                                       io_type='io_parallel',
                                                       output_dir='test_fix',
                                                       part='xcvu13p-flga2577-2-e',
                                                       clock_period=5)
hls_model.compile()

with the error message:

RuntimeError: Currently only support the case when input variables and input layer names match
Input layers = ['input_1', 'input_2'], input_vars = ['input_2', 'input_1']

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@jmduarte jmduarte added the bug label Dec 7, 2023
@jmduarte jmduarte requested a review from vloncar December 7, 2023 03:37
@jmduarte jmduarte added this to the v0.8.1 milestone Dec 7, 2023
@jmduarte jmduarte force-pushed the multi-input-order-fix branch 2 times, most recently from b66b45f to 9f1b0f1 Compare December 13, 2023 21:10
@jmduarte jmduarte force-pushed the multi-input-order-fix branch from c80c8f2 to 23b4fe7 Compare December 14, 2023 14:44
@jmitrevs
Copy link
Contributor

I think I like the way this fix works better than the old way. There is just the minor pre-commit issue that needs fixing.

@vloncar
Copy link
Contributor

vloncar commented Dec 15, 2023

I agree this looks good. A test would be nice, and I don't think it needs to be separate. We can piggyback on an existing test, for example here. Reversing in1 and in2 will cause failures without this fix. Along with the change perhaps a comment saying why was this done would help so we don't forget in the future.

@jmduarte
Copy link
Member Author

pre-commit.ci autofix

@jmitrevs jmitrevs merged commit 367090d into main Dec 15, 2023
5 checks passed
@jmitrevs jmitrevs deleted the multi-input-order-fix branch December 15, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants