Refactor and cleanup restartthinner #847

larsevj · 2026-01-16T15:02:36Z

No description provided.

codecov-commenter · 2026-01-16T15:13:09Z

Codecov Report

❌ Patch coverage is 98.36066% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.56%. Comparing base (cb65d1f) to head (633967e).

Files with missing lines	Patch %	Lines
src/subscript/restartthinner/restartthinner.py	98.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
+ Coverage   83.46%   83.56%   +0.09%     
==========================================
  Files          49       49              
  Lines        7293     7294       +1     
==========================================
+ Hits         6087     6095       +8     
+ 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.

Copilot

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.

src/subscript/restartthinner/restartthinner.py

Copilot · 2026-01-19T09:04:13Z

src/subscript/restartthinner/restartthinner.py

        String with path if found.

    Raises:
        IOError: if tool can't be found


The docstring states "IOError: if tool can't be found" but the function raises OSError. While IOError is an alias for OSError in Python 3, the docstring should be updated to match the actual exception being raised for consistency.

Suggested change

IOError: if tool can't be found

OSError: if tool can't be found

Copilot · 2026-01-19T09:04:14Z

src/subscript/restartthinner/restartthinner.py

+def date_slicer(
+    slicedates: list, restartdates: list, restartindices: list
+) -> list[int]:
    """Make a dict that maps a chosen restart date to a report index"""


The docstring still mentions "Make a dict that maps a chosen restart date to a report index" but the function now returns a list instead of a dict. The docstring should be updated to reflect that it returns a list of restart indices.

Suggested change

"""Make a dict that maps a chosen restart date to a report index"""

"""Return a list of restart indices closest to the requested slice dates."""

Copilot

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.

src/subscript/restartthinner/restartthinner.py

Copilot · 2026-01-19T09:56:13Z

src/subscript/restartthinner/restartthinner.py

+                )
+
+                # Move result back up
+                shutil.move(rstname, f"../{rstname}")


The path construction using string concatenation f"../{rstname}" is less robust and platform-agnostic than using Path operations. Consider using Path("..") / rstname instead for better portability and consistency with the rest of the codebase which uses Path objects.

Suggested change

shutil.move(rstname, f"../{rstname}")

shutil.move(rstname, Path("..") / rstname)

src/subscript/restartthinner/restartthinner.py

Copilot · 2026-01-19T09:56:13Z

src/subscript/restartthinner/restartthinner.py

+    with _working_directory(rstdir):
+        tempdir = Path(tempfile.mkdtemp(dir="."))
+        try:
+            # Move UNRST into temp directory and work there
+            shutil.move(rstname, tempdir / rstname)
+
+            with _working_directory(tempdir):
+                subprocess.run(
+                    [rd_unpack, rstname],
+                    capture_output=quiet,
+                    check=True,
+                )
+
+                for file in Path(".").glob("*.X*"):
+                    index = int(file.suffix.lstrip(".X"))
+                    if index not in slicerstindices:
+                        file.unlink()
+
+                remaining_files = sorted(Path(".").glob("*.X*"))
+                subprocess.run(
+                    [rd_pack, *[str(f) for f in remaining_files]],
+                    capture_output=quiet,
+                    check=True,
+                )
+
+                # Move result back up
+                shutil.move(rstname, f"../{rstname}")
+        finally:
+            shutil.rmtree(tempdir)


If an error occurs in rd_repacker after moving the original file into the temp directory but before successfully moving it back, the original file could be lost when the temp directory is cleaned up in the finally block. This is especially problematic when keep=False. Consider creating a temporary backup inside rd_repacker or ensuring the original file is only moved after successful processing, using a different approach like working on a copy.

Copilot · 2026-01-19T09:56:14Z

src/subscript/restartthinner/restartthinner.py

+                subprocess.run(
+                    [rd_unpack, rstname],
+                    capture_output=quiet,
+                    check=True,
+                )
+
+                for file in Path(".").glob("*.X*"):
+                    index = int(file.suffix.lstrip(".X"))
+                    if index not in slicerstindices:
+                        file.unlink()
+
+                remaining_files = sorted(Path(".").glob("*.X*"))
+                subprocess.run(
+                    [rd_pack, *[str(f) for f in remaining_files]],
+                    capture_output=quiet,
+                    check=True,
+                )


The subprocess.run calls use capture_output=quiet which will be either True or False. When True, both stdout and stderr are captured and discarded. When False, they are not captured. However, this means error messages from the subprocess will be hidden when quiet=True, making debugging difficult if the subprocess fails. Consider using stdout=subprocess.DEVNULL if quiet else None and always showing stderr for better error visibility, or at least including stderr in the exception message when check=True fails.

Copilot

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

Copilot

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.

src/subscript/restartthinner/restartthinner.py

tests/test_restartthinner.py

src/subscript/restartthinner/restartthinner.py

Copilot

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.

src/subscript/restartthinner/restartthinner.py

Copilot · 2026-01-19T11:20:14Z

src/subscript/restartthinner/restartthinner.py

    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


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.

src/subscript/restartthinner/restartthinner.py

Copilot · 2026-01-19T11:20:15Z

src/subscript/restartthinner/restartthinner.py

-    cwd = os.getcwd()
-    rstfilepath = Path(rstfilename).parent
-    tempdir = None
+def rd_repacker(rstfilename: str, slicerstindices: list[int], quiet: bool) -> None:


The parameter name "slicerstindices" appears to have a typo - it should likely be "slicedrstindices" or "sliced_rst_indices" for clarity. The "r" seems to be missing between "slice" and "st", making it read as "slicer-st-indices" rather than "sliced-rst-indices" (sliced restart indices).

src/subscript/restartthinner/restartthinner.py

tests/test_restartthinner.py

larsevj requested a review from Copilot January 19, 2026 09:01

larsevj marked this pull request as ready for review January 19, 2026 09:01

Copilot started reviewing on behalf of larsevj January 19, 2026 09:01 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

larsevj force-pushed the simplify_restartthinner branch from 56537b7 to 1b28dbe Compare January 19, 2026 09:36

larsevj requested review from Copilot January 19, 2026 09:51

Copilot started reviewing on behalf of larsevj January 19, 2026 09:51 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

larsevj force-pushed the simplify_restartthinner branch from 1b28dbe to 925b484 Compare January 19, 2026 10:06

larsevj requested a review from Copilot January 19, 2026 10:08

Copilot started reviewing on behalf of larsevj January 19, 2026 10:08 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

src/subscript/restartthinner/restartthinner.py Show resolved Hide resolved

tests/test_restartthinner.py Outdated Show resolved Hide resolved

src/subscript/restartthinner/restartthinner.py Show resolved Hide resolved

src/subscript/restartthinner/restartthinner.py Show resolved Hide resolved

Refactor and cleanup restartthinner

633967e

larsevj force-pushed the simplify_restartthinner branch from 925b484 to 633967e Compare January 19, 2026 11:05

larsevj requested a review from Copilot January 19, 2026 11:13

Copilot started reviewing on behalf of larsevj January 19, 2026 11:14 View session

Copilot AI reviewed Jan 19, 2026

View reviewed changes

	IOError: if tool can't be found
	OSError: if tool can't be found

	"""Make a dict that maps a chosen restart date to a report index"""
	"""Return a list of restart indices closest to the requested slice dates."""

	shutil.move(rstname, f"../{rstname}")
	shutil.move(rstname, Path("..") / rstname)

Refactor and cleanup restartthinner #847

Are you sure you want to change the base?

Refactor and cleanup restartthinner #847

Uh oh!

Conversation

larsevj commented Jan 16, 2026

Uh oh!

codecov-commenter commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jan 16, 2026 •

edited

Loading