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

Quartus multi out with stream fix #908

Merged

Conversation

calad0i
Copy link
Contributor

@calad0i calad0i commented Oct 31, 2023

A# Description

Quartus writer writes uncompilable programs when there are multiple outputs with io_stream. This PR fixes that.

Type of change

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

Tests

test/pytest/test_multiout_network.py

Test Configuration:

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • 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.

@calad0i calad0i mentioned this pull request Nov 7, 2023
7 tasks
@vloncar vloncar added this to the v0.8.1 milestone Nov 10, 2023
@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Nov 27, 2023
@jmitrevs
Copy link
Contributor

Does this work correctly if the multiple outputs have a different type? I am worried that they get forced to result_t, though I didn't check.

@jmitrevs
Copy link
Contributor

@vloncar
Copy link
Contributor

vloncar commented Nov 27, 2023

@jmitrevs Those lines were also problematic in our attempts to split the writing into multiple IPs. There's no practical reason for this feature to exist, it is merely to be human readable from the very ancient times, and in my (limited) tests everything still works if they are removed

@jmitrevs
Copy link
Contributor

jmitrevs commented Nov 27, 2023

I just tried:

def test_multi_clone(model, data, backend: str, io_type: str):
    output_dir = str(test_root_path / f'hls4mlprj_multiout_network_{backend}_{io_type}')
    hls_config = config_from_keras_model(model, "name", default_precision='fixed<32,5>')
    hls_config["LayerName"]["dense_linear"]["Precision"]["result"] = 'fixed<35,5>'
    hls_config["LayerName"]["dense_1_linear"]["Precision"]["result"] = 'fixed<40,5>'
    hls_config["LayerName"]["dense"]["Precision"]["result"] = 'fixed<35,5>'
    hls_config["LayerName"]["dense_1"]["Precision"]["result"] = 'fixed<40,5>'
    model_hls = convert_from_keras_model(
        model, backend=backend, output_dir=output_dir, hls_config=hls_config, io_type=io_type
    )
    model_hls.compile()
    r_hls = model_hls.predict(data)
    r_keras = [x.numpy() for x in model(data)]

    assert np.allclose(r_hls[0], r_keras[0], atol=1e-5, rtol=0)
    assert np.allclose(r_hls[1], r_keras[1], atol=1e-5, rtol=0)

and both outputs are result_t of fixed<35,5>. I could have made a mistake in the test code (and I am not sure why I had to define the linear result like that, too), but the behavior doesn't seem quire right.

@calad0i
Copy link
Contributor Author

calad0i commented Dec 2, 2023

I changed the two places in graph.py, and now the output variables are not overwriting each other. It does not seem to affect other tests in my tests. Though, maybe this should go into a separate PR.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 4, 2023
@jmitrevs
Copy link
Contributor

jmitrevs commented Dec 4, 2023

There is still some manual seeding left, if I looked at the code correctly.

@jmitrevs
Copy link
Contributor

@calad0i, can you remove the remaining seed setting?

jmitrevs
jmitrevs previously approved these changes Dec 14, 2023
@jmitrevs jmitrevs dismissed their stale review December 14, 2023 20:26

Actually, let me run pytests first again. So hold off on the merge.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 14, 2023
@jmitrevs
Copy link
Contributor

Generally other that the test_root_path issue, I think this is good. After that is fixed we can merge it.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Dec 15, 2023
@jmitrevs jmitrevs merged commit 9be5cba into fastmachinelearning:main Dec 15, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants