Skip to content

Add extra test for params #2051

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

Merged
merged 2 commits into from
Aug 28, 2022
Merged

Add extra test for params #2051

merged 2 commits into from
Aug 28, 2022

Conversation

christiangnrd
Copy link
Contributor

Adding the test suggested by @mcabbott in pull-request #2048 to prevent future regression.

If there are any more cases that you think should be added, let me know.

I made a separate pull-request because I feel that this test should be added regardless of the stats of #2048.

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

I don't love size.(Flux.params(m)) as I'm not sure the order is guaranteed, but lots of other tests above do the same.

Maybe the comment can be clearer too, otherwise fine, thanks!

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>
@christiangnrd
Copy link
Contributor Author

I used size.(Flux.params(m)) because all the other tests do the same. I agree about the comment being vague, l wrote it as specific as I could given my current familiarity with the code :)

@mcabbott mcabbott merged commit 39fe9f9 into FluxML:master Aug 28, 2022
@christiangnrd christiangnrd deleted the new-params-tests branch August 28, 2022 19:36
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