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

modify sim_ber callback to include all results #286

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Conversation

nbecker
Copy link
Contributor

@nbecker nbecker commented Dec 12, 2023

Description

Pass all stats to sim_ber callback so they can be used to log results for all es/no points

  • Fixes a bug?

Describe what was wrong, and explain the solution in detail.

  • Adds a new feature?

Describe the new feature, ensure that it is properly documented. Note that for new features unit tests are required.

  • Introduces API changes?

Address in detail why it is needed. Explain and describe the consequences, does it break older code? Note that API changes might be delayed until a major bump release for inclusion.

  • Other contributions

Please detail the nature of the submission.

Checklist

[ ] Detailed description
[ ] Added references to issues and discussions
[ ] Added / modified documentation as needed
[ ] Added / modified unit tests as needed
[ ] Passes all tests
[ ] Lint the code
[ ] Performed a self review
[ ] Ensure you Signed-off the commits. Required to accept contributions!
[ ] Co-authored with someone? Add Co-authored-by: user@domain and ensure they signed off their commits too.

@SebastianCa
Copy link
Collaborator

Thank you for this PR.

I have a two open points:

  • We need to add the current snr_index as additional input to the callback to know which SNR point is currently simulated
  • Please align the docstring of sim_ber accordingly

@nbecker
Copy link
Contributor Author

nbecker commented Dec 13, 2023

I've made the requested changes. Is the PR now complete? I am new to git, not sure if I need to take any further action.

@SebastianCa
Copy link
Collaborator

I did some minor edits, the PR is now ready. We will merge into the next release of Sionna.

Thank you for your contribution.

@gmarcusm
Copy link
Collaborator

gmarcusm commented Jan 9, 2024

Hi @nbecker , thanks for the contributions. Can you please reply here in the conversation with the code signoff? I can then add it to the merge commit.

@nbecker
Copy link
Contributor Author

nbecker commented Jan 9, 2024

I've done
git commit --amend --signoff
and
git push

Hopefully everything is good now?

@gmarcusm
Copy link
Collaborator

gmarcusm commented Jan 9, 2024

thanks for the prompt reply. Unfortunately, the commit does not seems to be signed-off:

commit 6e112a9e19d1233000f66019b375fc44dee97d25 (HEAD -> main, origin/main, origin/HEAD)
Merge: cf745b4 be78a0d
Author: ndbecker <[email protected]>
Date:   Tue Jan 9 07:04:47 2024 -0500

    Merge branch 'main' into main

but do not worry. If you just paste the sign-off here I will add it.

@nbecker
Copy link
Contributor Author

nbecker commented Jan 9, 2024 via email

@gmarcusm gmarcusm merged commit 1be7e79 into NVlabs:main Jan 10, 2024
4 checks passed
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.

3 participants