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

[BBPBGLIB-1153] Allow disabling reports from SONATA config #156

Closed
wants to merge 3 commits into from
Closed

[BBPBGLIB-1153] Allow disabling reports from SONATA config #156

wants to merge 3 commits into from

Conversation

kbvw
Copy link
Collaborator

@kbvw kbvw commented Apr 19, 2024

Context

This PR solves
https://bbpteam.epfl.ch/project/issues/browse/BBPBGLIB-1153

According to the SONATA specification, reports can be disabled with an optional enabled parameter in the config file. This option was already parsed, but not used to disable the reports.

Scope

A small change to the enable_reports method of the Node class, filtering the reports based on the enabled flag.

Testing

An integration test in tests/integration-e2e/test_reports.py, reusing part of the existing fixture there and reading the flag from a SONATA config file.

Review

  • PR description is complete
  • Coding style (imports, function length, New functions, classes or files) are good
  • Unit/Scientific test added
  • Updated Readme, in-code, developer documentation

@kbvw
Copy link
Collaborator Author

kbvw commented Apr 19, 2024

The test is admittedly not completely end-to-end, but given the other tests there this seemed to me the most logical place to include it. Let me know if I should move it somewhere else.

Copy link
Collaborator

@jorblancoa jorblancoa left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I like the solution, just a couple of nitpicks! :)

Copy link
Collaborator

@WeinaJi WeinaJi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@WeinaJi
Copy link
Collaborator

WeinaJi commented Apr 22, 2024

@kbvw , this PR can not trigger our GitLab CI tests because it is requested from the forked repo not the repo branch.
Have you been added to the BBP GH group ? If yes, can you open a PR from a neurodamus branch with the same commit? Sorry about that.

@kbvw
Copy link
Collaborator Author

kbvw commented Apr 22, 2024

No worries, that's my bad. I just now got added: will open a new one.

@kbvw
Copy link
Collaborator Author

kbvw commented Apr 22, 2024

Closing in favour of #159.

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