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.

Sign up for GitHub

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

Jump to bottom

Sass/bulk observables #321

Merged
merged 10 commits into from
Dec 11, 2024
Merged

Sass/bulk observables #321

merged 10 commits into from
Dec 11, 2024

Conversation

nilssass
Copy link
Collaborator

No description provided.

@nilssass nilssass requested a review from Hendrik1704 November 22, 2024 16:03
@nilssass nilssass changed the base branch from main to sparkx_devel November 22, 2024 16:04
@Hendrik1704 Hendrik1704 added this to the SPARKX 2.0 milestone Nov 22, 2024
@Hendrik1704 Hendrik1704 added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 22, 2024
Hendrik1704
Hendrik1704 requested changes Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@Hendrik1704 Hendrik1704 left a comment

Choose a reason for hiding this comment

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

I have a few things that need to be addressed:

  • There is no documentation for the new class and the documented functions have a different documentation format.
  • Some default values for ranges seem strange (e.g. dN/deta).
  • Some spelling mistakes

src/sparkx/BulkObservables.py Show resolved Hide resolved
src/sparkx/BulkObservables.py Show resolved Hide resolved
src/sparkx/BulkObservables.py Outdated Show resolved Hide resolved
src/sparkx/BulkObservables.py Show resolved Hide resolved
src/sparkx/BulkObservables.py Outdated Show resolved Hide resolved
src/sparkx/BulkObservables.py Outdated Show resolved Hide resolved
tests/test_BulkObservables.py Outdated Show resolved Hide resolved
tests/test_BulkObservables.py Outdated Show resolved Hide resolved
tests/test_BulkObservables.py Outdated Show resolved Hide resolved
tests/test_BulkObservables.py Outdated Show resolved Hide resolved
@Hendrik1704 Hendrik1704 self-requested a review December 11, 2024 15:16
@NGoetz NGoetz merged commit e735d31 into sparkx_devel Dec 11, 2024
3 checks passed
@NGoetz NGoetz deleted the sass/BulkObservables branch December 11, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hendrik1704 Hendrik1704 Hendrik1704 approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
SPARKX 2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nilssass @Hendrik1704 @NGoetz