Skip to content

Refactor and cleanup restartthinner#847

Open
larsevj wants to merge 1 commit intomainfrom
simplify_restartthinner
Open

Refactor and cleanup restartthinner#847
larsevj wants to merge 1 commit intomainfrom
simplify_restartthinner

Conversation

@larsevj
Copy link
Collaborator

@larsevj larsevj commented Jan 16, 2026

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.52%. Comparing base (4ac49d5) to head (eb3e265).

Files with missing lines Patch % Lines
src/subscript/restartthinner/restartthinner.py 98.11% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@larsevj larsevj requested a review from Copilot January 19, 2026 09:01
@larsevj larsevj marked this pull request as ready for review January 19, 2026 09:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 with subprocess.run() for better error handling and portability
  • Refactored find_resdata_app() to use shutil.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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 144 to +167
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
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
@larsevj larsevj force-pushed the simplify_restartthinner branch 2 times, most recently from 73c3fa3 to c172ddf Compare January 26, 2026 10:09
@larsevj larsevj requested a review from Copilot January 26, 2026 10:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.

@larsevj larsevj requested a review from alifbe January 26, 2026 10:11
@larsevj larsevj force-pushed the simplify_restartthinner branch from c172ddf to eb3e265 Compare February 5, 2026 11:54
@larsevj larsevj requested a review from Copilot February 5, 2026 12:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

Use assert not caplog.text instead of assert len(caplog.text) == 0 for a more Pythonic check of empty strings.

Suggested change
assert len(caplog.text) == 0
assert not caplog.text

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

..

@larsevj larsevj requested review from alifbe and removed request for alifbe February 5, 2026 12:16
@larsevj larsevj requested a review from rnyb February 5, 2026 12:16
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.

2 participants