Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
==========================================
+ Coverage 83.44% 83.52% +0.07%
==========================================
Files 49 49
Lines 7287 7279 -8
==========================================
- Hits 6081 6080 -1
+ Misses 1206 1199 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors and modernizes the restartthinner module by improving code quality, replacing deprecated patterns, and enhancing maintainability.
Changes:
- Replaced
os.system()calls withsubprocess.run()for better error handling and portability - Refactored
find_resdata_app()to useshutil.which()instead of manual PATH traversal - Simplified
date_slicer()to return a list directly instead of a dict, improving clarity
Comments suppressed due to low confidence (1)
src/subscript/restartthinner/restartthinner.py:69
- There are two spelling errors in this comment: "dont't" should be "don't" and "remainding" should be "remaining".
First unpacking a UNRST file, then deleting dates the dont't want, then
pack the remainding files into a new UNRST file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
56537b7 to
1b28dbe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1b28dbe to
925b484
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
925b484 to
633967e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if numberofslices > 1: | ||
| slicedates = pandas.DatetimeIndex( | ||
| numpy.linspace( | ||
| pandas.Timestamp(restart_dates[0]).value, | ||
| pandas.Timestamp(restart_dates[-1]).value, | ||
| slicedates = pd.DatetimeIndex( | ||
| np.linspace( | ||
| pd.Timestamp(restart_dates[0]).value, | ||
| pd.Timestamp(restart_dates[-1]).value, | ||
| int(numberofslices), | ||
| ) | ||
| ).to_list() | ||
| else: | ||
| slicedates = [restart_dates[-1]] # Only return last date if only one is wanted | ||
|
|
||
| slicerstindices = list( | ||
| date_slicer(slicedates, restart_dates, restart_indices).values() | ||
| ) | ||
| slicerstindices.sort() | ||
| slicerstindices = list(set(slicerstindices)) # uniquify | ||
| slicerstindices = date_slicer(slicedates, restart_dates, restart_indices) | ||
| slicerstindices = sorted(set(slicerstindices)) # uniquify |
There was a problem hiding this comment.
When numberofslices equals the number of existing restart points, the date_slicer logic may still try to calculate evenly-spaced dates which could theoretically select duplicates. The sorted(set(...)) on line 179 handles this, but it means the user might get fewer restart points than requested. Consider adding a check or warning when numberofslices >= len(restart_indices) to inform the user that all restart points are being kept.
73c3fa3 to
c172ddf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c172ddf to
eb3e265
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| restartthinner.main() | ||
|
|
||
| # Check that log output is empty | ||
| assert len(caplog.text) == 0 |
There was a problem hiding this comment.
Use assert not caplog.text instead of assert len(caplog.text) == 0 for a more Pythonic check of empty strings.
| assert len(caplog.text) == 0 | |
| assert not caplog.text |
No description provided.