Skip to content

Add decimals parameter to sc.round function #3793

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
Merged
MridulS merged 7 commits into from
Dec 2, 2025
Merged

Add decimals parameter to sc.round function #3793

MridulS merged 7 commits into from
Dec 2, 2025

Conversation

Copy link
Contributor

Copilot AI commented Nov 26, 2025 •
edited
Loading

  • Analyze the existing sc.round function in /src/scipp/core/math.py
  • Review numpy's np.round API signature (accepts decimals parameter)
  • Review existing tests in /tests/core/math_test.py
  • Modify sc.round function to accept decimals parameter
  • Update the DataGroup.round method to pass the decimals parameter
  • Update type stubs in cpp_classes.pyi
  • Add tests for the new decimals parameter
  • Run linter to validate changes
  • Run code review and address feedback
  • Run CodeQL security scan (no alerts found)
  • Document overflow behavior in docstring
  • Fix mypy type ignore comments
Original prompt

This section details on the original issue you should resolve

<issue_title>Add decimals to sc.round function</issue_title>
<issue_description>It would be useful to be able to round to d decimals without resorting to something hard to read and error prone like:

sc.round(10**d * var) / 10**d

Numpy allows a decimals argument to np.round, with that the above becomes:

np.round(var, d)
```</issue_description>

<agent_instructions>Add tests, look at https://numpy.org/devdocs//reference/generated/numpy.round.html for API signature.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits November 26, 2025 18:25
@MridulS
Add decimals parameter to sc.round function
08ceae3 
Co-authored-by: MridulS <5363860+MridulS@users.noreply.github.com>
@MridulS
Simplify round function implementation for better clarity
cec8dca 
Co-authored-by: MridulS <5363860+MridulS@users.noreply.github.com>
Copilot AI changed the title [WIP] Add decimals to sc.round function Add decimals parameter to sc.round function Nov 26, 2025
Copilot AI requested a review from MridulS November 26, 2025 18:30
SimonHeybrock
SimonHeybrock reviewed Nov 27, 2025
View reviewed changes
src/scipp/core/math.py
Comment on lines +404 to +405
factor = 10**decimals
result = _call_cpp_func(_cpp.rint, factor * x) / factor
Copy link
Member

@SimonHeybrock SimonHeybrock Nov 27, 2025

Choose a reason for hiding this comment

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

I'd be worried about overflows here.

Copy link
Member

@MridulS MridulS Nov 27, 2025

Choose a reason for hiding this comment

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

@copilot what do you think?

Copy link
Contributor Author

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

Valid concern. The implementation uses rint(x * 10^d) / 10^d, which can indeed overflow for very large values combined with large positive decimals. However, this is the same algorithm that NumPy uses (as documented in their docstring), and they accept this limitation.

I've added a note in the docstring (7560eb4) to document this behavior explicitly, referencing that it matches NumPy's approach.

@MridulS
Document potential overflow behavior for large values with decimals
7560eb4 
Co-authored-by: MridulS <5363860+MridulS@users.noreply.github.com>
@MridulS
Copy link
Member

MridulS commented Nov 27, 2025

@copilot I see these errors in the CI:

mypy: commands[0]> python -m mypy . --exclude '^src/'
install/scipp/core/math.py:414: error: Unused "type: ignore" comment  [unused-ignore]
install/scipp/core/math.py:414: error: Returning Any from function declared to return "_T"  [no-any-return]
install/scipp/core/math.py:414: note: Error code "no-any-return" not covered by "type: ignore" comment
Found 2 errors in 1 file (checked 99 source files)
mypy: exit 1 (14.01 seconds) /home/runner/work/scipp/scipp> python -m mypy . --exclude '^src/' pid=17609

@MridulS
Fix mypy type ignore comment for return statement
2ef6bb5 
Co-authored-by: MridulS <5363860+MridulS@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 27, 2025

@copilot I see these errors in the CI:

...

Fixed in 2ef6bb5. The type ignore comment was using return-value but mypy was reporting no-any-return. Changed to the correct error code.

@MridulS MridulS marked this pull request as ready for review November 27, 2025 09:10
@MridulS MridulS enabled auto-merge (squash) November 28, 2025 06:51
@MridulS MridulS requested a review from SimonHeybrock December 1, 2025 13:59
@MridulS MridulS merged commit f606d22 into main Dec 2, 2025
4 checks passed
@MridulS MridulS deleted the copilot/add-decimals-to-round-function branch December 2, 2025 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SimonHeybrock SimonHeybrock SimonHeybrock approved these changes

+1 more reviewer

@MridulS MridulS MridulS approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MridulS MridulS

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add decimals to sc.round function

3 participants

@MridulS @SimonHeybrock