Added date argument in prtvol2csv (#712)#850
Conversation
lilbe66
commented
Feb 13, 2026
lilbe66
commented
- Added date argument to prtvol2csv.py, to extract volumes from PRT file at the specified date
- Included function to search for report date in PRT file
- Check that a volume report exists at the specified date
- Check that the date provided has ISO 8601 format, or one of the strings "first" and "last"
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
==========================================
+ Coverage 83.52% 83.59% +0.06%
==========================================
Files 49 49
Lines 7279 7331 +52
==========================================
+ Hits 6080 6128 +48
- Misses 1199 1203 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request adds functionality to extract volumes from PRT files at a specified date by introducing a --date argument to the prtvol2csv tool. The changes allow users to specify either an ISO-8601 formatted date (YYYY-MM-DD) or the strings "first" or "last" to select which BALANCE report to extract volumes from.
Changes:
- Added date argument support with validation for ISO-8601 format or "first"/"last" keywords
- Modified function signatures to return additional date information
- Added helper functions for date parsing and validation from PRT files
- Enhanced test coverage with new test cases and test data
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src/subscript/prtvol2csv/prtvol2csv.py | Core implementation: added date parameter, date parsing functions, and modified return types to include date information |
| tests/test_prtvol2csv.py | Added comprehensive tests for date parsing, validation, and command-line usage with date argument |
| src/subscript/rmsecl_volumetrics/rmsecl_volumetrics.py | Updated to handle new tuple return type from currently_in_place_from_prt |
| tests/testdata_prtvol2csv/DROGON_NOBAL_FLOW.PRT | New test data file for Flow PRT files without BALANCE reports |
| tests/testdata_prtvol2csv/0readme.txt | Documentation of test data files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def reservoir_volumes_from_prt( | ||
| prt_file: str, | ||
| dates_bal_report: any, |
There was a problem hiding this comment.
The type hint "any" should be capitalized as "Any" and imported from typing. Additionally, "dates_bal_report" should have a more precise type annotation like "np.ndarray" since it's used as a numpy array throughout the function.
| prt_file: str, fipname: str = "FIPNUM", date: str | None = None | ||
| ) -> pd.DataFrame: | ||
| prt_file: str, fipname: str = "FIPNUM", date_str: str | None = None | ||
| ) -> tuple[pd.DataFrame, np.array, str]: |
There was a problem hiding this comment.
The return type annotation "np.array" should be "np.ndarray" for consistency with NumPy's type hints. The type "np.array" is a function, not a type.
| ) | ||
| parser.add_argument( | ||
| "--date", | ||
| # type=lambda d: datetime.strptime(d, "%Y-%m-%d").date(), |
There was a problem hiding this comment.
The commented-out type annotation should either be uncommented and used, or removed entirely. Type annotations with lambda functions can be tricky, but if this approach was problematic, the comment should be removed to keep the code clean.
| # type=lambda d: datetime.strptime(d, "%Y-%m-%d").date(), |
|
|
||
|
|
||
| def date_string_format_check(date_str: str) -> bool: | ||
| """Check if a date string is in ISO 8601 format 'YYY-MM-DD'""" |
There was a problem hiding this comment.
There's a typo in the docstring: "YYY-MM-DD" should be "YYYY-MM-DD" (missing one Y).
| """Check if a date string is in ISO 8601 format 'YYY-MM-DD'""" | |
| """Check if a date string is in ISO 8601 format 'YYYY-MM-DD'""" |
| def currently_in_place_from_prt( | ||
| prt_file: str, fipname: str = "FIPNUM", date: str | None = None | ||
| ) -> pd.DataFrame: | ||
| prt_file: str, fipname: str = "FIPNUM", date_str: str | None = None | ||
| ) -> tuple[pd.DataFrame, np.array, str]: |
There was a problem hiding this comment.
This is a breaking change to the function signature. The function now returns a tuple instead of just a DataFrame, which will break existing code that calls this function. Consider either:
- Maintaining backward compatibility by having a parameter that controls whether to return the extended tuple or just the DataFrame
- Creating a new function with a different name for the extended functionality
- Documenting this as a breaking change in the release notes
The change in rmsecl_volumetrics.py (line 151) shows that this breaks at least one internal caller, and there may be other external users of this function.
| date (str): If None or "first", the first date will be used. If not None, | ||
| "first" or "last", it should be an ISO-formatted date string | ||
| to extract (YYYY-MM-DD). |
There was a problem hiding this comment.
The docstring still refers to the parameter as "date" but the actual parameter name is now "date_str". The documentation should be updated to match the parameter name.
| date (str): If None or "first", the first date will be used. If not None, | |
| "first" or "last", it should be an ISO-formatted date string | |
| to extract (YYYY-MM-DD). | |
| date_str (str): If None or "first", the first date will be used. If not None, | |
| "first" or "last", it should be an ISO-formatted date string | |
| to extract (YYYY-MM-DD). |
| Args: | ||
| prt_file (str): PRT filename | ||
|
|
||
| Returns: | ||
| pd.DataFrame |
There was a problem hiding this comment.
The docstring lacks a description of the new parameters "dates_bal_report" and "date_str" in the Args section. The function signature was extended but the documentation wasn't updated to describe what these new parameters are for.
| if date not in available_dates: | ||
| logger.error( | ||
| f" The user specified date {date_str} is not available " | ||
| f"with BALANCE report in the PRT file.\n" | ||
| f"Available dates with BALANCE report are:\n{available_dates_str}" | ||
| ) | ||
| return inplace_df, np.array([]), date_str |
There was a problem hiding this comment.
When a user-specified date is not found in the available dates, the function logs an error and returns early with an empty tuple element. However, the calling code in main() (line 475-476) only checks if simvolumes_df is empty, but doesn't validate that available_dates and selected_date are meaningful. This could lead to issues if the code tries to use available_dates or selected_date after this error. Consider either raising an exception or ensuring that the early return values are handled more consistently in the calling code.
| "--date", | ||
| # type=lambda d: datetime.strptime(d, "%Y-%m-%d").date(), | ||
| help=( | ||
| "Specify a valid date for existing BALANCE report in the PRT file." |
There was a problem hiding this comment.
The help text is missing a space after the first sentence. It should read: "Specify a valid date for existing BALANCE report in the PRT file. The date must be in ISO-8601 format..."
| "Specify a valid date for existing BALANCE report in the PRT file." | |
| "Specify a valid date for existing BALANCE report in the PRT file. " |
| date_found = True | ||
| if isinstance(date_object, date) and date_object > wanted_date: | ||
| # Next report step (i.e. no Reservoir Volume Report) -> stop search | ||
| date_found = False # Not neccessary, but to speed up |
There was a problem hiding this comment.
Variable date_found is not used.
| date_found = False # Not neccessary, but to speed up |
2 participants
