Conversation
…me_split_to_pp.py
The ending date should be June 30
from fre-cli instead of fre-workflows
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #717 +/- ##
==========================================
+ Coverage 84.22% 84.24% +0.01%
==========================================
Files 70 71 +1
Lines 4773 4962 +189
==========================================
+ Hits 4020 4180 +160
- Misses 753 782 +29
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@copilot open a PR to this branch and:
|
Co-authored-by: ilaflott <[email protected]>
Co-authored-by: ilaflott <[email protected]>
Co-authored-by: ilaflott <[email protected]>
reduce `rename-split` test-data to minimal possible size
ilaflott
left a comment
ilaflott
left a comment
There was a problem hiding this comment.
a bunch of this is nit picking about style, this looks largely functional. i'll ponder approving when it's taken out of draft
This is needed for input files with one timestep and no time bounds.
- Proper docstrings - Module import cleanup - Indent cleanup - Other: remove unneeded environment variables
| duration_object = duration_parser.parse(duration) | ||
| date2 = date1 + duration_object - one_month | ||
| format_ = "%Y" | ||
| freq_label = duration |
There was a problem hiding this comment.
Reviewers, this elif section that assumes a history file's frequency based on its name (i.e. containing "annual") is distasteful.
The just-above "if diag_manifest exists" will be the long-term approach.
But until diag manifests are safely assumed to be available, I think this temporary hard-coding is worth it.
|
I think this is completely ready now |
ilaflott
left a comment
ilaflott
left a comment
There was a problem hiding this comment.
ready to approve pending response to my comment on fre/yamltools/info_parsers/pp_info_parser.py. the other comments i have are nits that are style-oriented, or have to do with untested try/except lines that don't generally give me anxiety
| def test_get_duration_from_two_dates(): | ||
| """ | ||
| Lookup some durations between two dates | ||
| """ | ||
| assert "P1M" == get_duration_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2009, 2, 1)) | ||
| assert "P6M" == get_duration_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2009, 7, 1)) | ||
| assert "P1Y" == get_duration_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2010, 1, 1)) | ||
| assert "P5Y" == get_duration_from_two_dates(cftime.datetime(2000, 1, 1), cftime.datetime(2005, 1, 1)) |
There was a problem hiding this comment.
nit, could be parameterize cases via pytest
| yaml_content_str += settings_content | ||
| fre_logger.info(" settings yaml: %s", settings) | ||
| else: | ||
| fre_logger.info(" settings yaml: not used") |
There was a problem hiding this comment.
this decision branch being untested is the one that worries me the most currently- especially since we're going toward no-settings-yaml. is this because the schema isn't updated yet?
There was a problem hiding this comment.
It is a large necessary change that @singhd789 is making to the yaml parsing and combining methods, both to standardize the methods and to use uw config compose as the primary combining mechanism.
There will be plenty of settings yamls everywhere, in that any yaml piece can be a reusable settings yaml. But these settings yamls will be schema-controlled based on the application schema, not a top-level "settings" schema.
From the meeting today I think we almost agreed :)
There was a problem hiding this comment.
that was a lot more than i bargained for! I was mostly trying to figure out how permissive to be on the untested-lines front. since many are hashing this out in a very thoughful manner elsewhere, trying to account for a myriad of concerns beyond the scope of the repo, i'll let it slide as a nit
|
I'm a bit more concerned about some of the cuts that it's made to the test
files but I can't comment about it yet because I'm having trouble logging
into github.
…On Wed, Mar 18, 2026 at 11:00 AM Ian L. ***@***.***> wrote:
***@***.**** commented on this pull request.
ready to approve pending response to my comment on
fre/yamltools/info_parsers/pp_info_parser.py. the other comments i have
are nits that are style-oriented, or have to do with untested try/except
lines that don't generally give me anxiety
------------------------------
In fre/pp/rename_split_script.py
<#717 (comment)>:
> +import logging
+import os
+import shutil
+import pathlib
+from pathlib import Path
+import xarray as xr
+import cftime
+from datetime import timedelta
+import yaml
+from metomi.isodatetime.parsers import DurationParser, TimePointParser
nit, import order!
# stdlib
# 3rd party
# internal
------------------------------
In fre/pp/tests/test_rename_split_to_pp.py
<#717 (comment)>:
> +# Third-party imports
+import cftime
+import pytest
+
+# Local application imports
+from fre.pp.rename_split_script import (
+ get_duration_from_two_dates,
+ get_freq_and_format_from_two_dates,
+ rename_split
+)
+
+def test_get_freq_and_format_from_two_dates():
+ """
+ Lookup some frequencies and formats between 2 dates
+ """
+ assert ("P1Y", "%Y") == get_freq_and_format_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2010, 1, 1))
nit, could be parameterize cases via pytest
------------------------------
In fre/pp/tests/test_rename_split_to_pp.py
<#717 (comment)>:
> +def test_get_duration_from_two_dates():
+ """
+ Lookup some durations between two dates
+ """
+ assert "P1M" == get_duration_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2009, 2, 1))
+ assert "P6M" == get_duration_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2009, 7, 1))
+ assert "P1Y" == get_duration_from_two_dates(cftime.datetime(2009, 1, 1), cftime.datetime(2010, 1, 1))
+ assert "P5Y" == get_duration_from_two_dates(cftime.datetime(2000, 1, 1), cftime.datetime(2005, 1, 1))
nit, could be parameterize cases via pytest
------------------------------
On fre/pp/rename_split_script.py
<#717 (comment)>:
nit(s), certain uncovered exceptions in here could easily be covered
------------------------------
In fre/yamltools/info_parsers/pp_info_parser.py
<#717 (comment)>:
> former_log_level = fre_logger.level
fre_logger.setLevel(logging.INFO)
- fre_logger.info(" settings yaml: %s", settings)
+
+ # if settings yaml is used, apply it
+ if settings is not None:
+ with open(f"{self.mainyaml_dir}/{settings}", 'r') as f:
+ settings_content = f.read()
+ yaml_content_str += settings_content
+ fre_logger.info(" settings yaml: %s", settings)
+ else:
+ fre_logger.info(" settings yaml: not used")
this decision branch being untested is the one that worries me the most
currently- especially since we're going toward no-settings-yaml. is this
because the schema isn't updated yet?
—
Reply to this email directly, view it on GitHub
<#717 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC362WXJRAHP5B7VDAKNFDD4RK2YJAVCNFSM6AAAAACTYPXZR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTSNRYGE2DKMZQHE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
Carolyn Whitlock
|
|
|
|
it'd also be good to make sure the pipeline passes after you fix conflicts and update via |
What changed: Converted 6 inline assertions into a parametrized test with 6 test cases Each case now runs as a separate test with clear identifiers like test_get_freq_and_format_from_two_dates[P1Y-%Y] Cleaner output: if one case fails, others still run and report independently Benefits: Improved test clarity and failure reporting Each test case shows exactly which parameters failed Easier to add new test cases in the future Follows pytest best practices refactored test_get_duration_from_two_dates using the same @pytest.mark.parametrize approach. The 4 test cases now run independently, making it easy to identify which specific duration calculation fails if any of them do.
New exception tests: test_get_freq_and_format_from_two_dates_raises_valueerror (parameterized, 2 cases) Tests ValueError when frequency cannot be determined from dates test_get_duration_from_two_dates_raises_valueerror (parameterized, 2 cases) Tests ValueError when duration cannot be determined from dates test_rename_split_raises_filenotfounderror_no_files Tests FileNotFoundError when no files matching the component are found test_rename_file_raises_valueerror_bad_filename Tests ValueError when filename doesn't match the expected format test_rename_file_raises_filenotfounderror_missing_diag_manifest Tests FileNotFoundError when a non-existent diag manifest path is provided All tests use pytest.raises() with appropriate error message matching. The file has no syntax errors and the tests follow the same patterns as the existing parameterized tests.
Describe your changes
Conversion of fre-workflows rename-split shell script into fre-cli python. Preserved existing rename-split test cases.
Issue ticket number and link (if applicable)
Checklist before requesting a review