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

Adding GarNet examples + config-files and data directories #4

Merged
merged 2 commits into from
Aug 19, 2020

Conversation

yiiyama
Copy link
Contributor

@yiiyama yiiyama commented Aug 19, 2020

To be merged after hls4ml PR 213 is merged.
Although the examples would not convert / build without hls4ml PR 213, let's merge this PR first because 213 uses one of the new examples for build tests.

@yiiyama
Copy link
Contributor Author

yiiyama commented Aug 19, 2020

Or perhaps right before it is merged, because I'm adding a test line that uses the 1layer model.

@Duchstf
Copy link
Member

Duchstf commented Aug 19, 2020

Hm ... this actually might complicate things a little bit. Because of a few things:

  1. The current example-models submodule in hls4ml is tagged at an older commit. So if you want to add the test you need to move the tag to the latest example-models version in master.

  2. However, it might mess up other tests, since there have been some minor changes in model file names here. But I'll look into this, I think it should be easy to fix. Do you know when the Garnet PR is planned to be merged?

@Duchstf
Copy link
Member

Duchstf commented Aug 19, 2020

Actually, regarding point 2, I don't think there would be any problem now that I looked into the test. So you just have to move the tag 😁.

Also, maybe we can merge this PR first, and then I'll make some changes to the fetching examples scheme (should be quick) to your PR 213, then we can merge it to hls4ml. But it depends on your timeline, if you prefer to merge PR 213 before being able to fetch the garnet examples, then it's fine too. I might just make a separate PR after yours to support downloading them.

@yiiyama
Copy link
Contributor Author

yiiyama commented Aug 19, 2020

Thanks for the comments @Duchstf ! I think it's better to merge this PR first before 213 (despite my original comment in this PR - which I edited now).

Comment on lines +1 to +2
KerasJson: garnet_3layer.json
KerasH5: garnet_3layer_weights.h5
Copy link
Member

Choose a reason for hiding this comment

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

I think everything looks good now. I just have a question here. If you do end up using the config file in your test, wouldn't you use relative path here? Or is it not important?

Copy link
Member

Choose a reason for hiding this comment

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

If you are fine leaving them like this I can merge this PR now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think it's fine like this. These lines are sort of suggesting that the user should copy all relevant files into a work directory before running hls4ml convert.

@Duchstf Duchstf merged commit c08ecdb into fastmachinelearning:master Aug 19, 2020
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