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 82.76% 84.16% +1.40%
==========================================
Files 68 71 +3
Lines 4525 4933 +408
==========================================
+ Hits 3745 4152 +407
- Misses 780 781 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| nc_exists = [osp.isfile(el) for el in nc_files] | ||
| assert all(nc_exists) | ||
|
|
||
| def test_rename_split_to_pp_multiply(): |
|
@copilot open a PR to this branch and:
|
Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
Co-authored-by: ilaflott <6273252+ilaflott@users.noreply.github.com>
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
| from os import path as osp | ||
| import pprint | ||
| from pathlib import Path |
There was a problem hiding this comment.
What's the difference between os.path and pathlib? Are they both needed or can you just use pathlib?
There was a problem hiding this comment.
I'm sure we could use pathlib only, you're right. I'll adjust.
| from . import status_script | ||
| from . import wrapper_script | ||
| from . import split_netcdf_script | ||
| from . import rename_split_script |
There was a problem hiding this comment.
Not a big thing, just curious (had the same question about split netcdf at one point) - Should this subtool go in fre/app? Just asking because I think that's how we started regrid and remap rewrites (putting them in fre/app vs fre/pp). I mean if it makes more sense being here, all good.
Maybe we should distinguish between fre/app and fre/pp subtools a bit better. In my head they're a teeny bit jumbled for the rewrites
There was a problem hiding this comment.
Completely jumbled, I agree. Some of the fre pp stuff that didn't fit well will go to your fre workflow tools at least.
| """ | ||
| Accept two dates and returns frequency string in ISO8601 and associated format string. | ||
| """ |
There was a problem hiding this comment.
Reminder to add in the in line doc:
:param date1: ...
:type date1: ...
...
:return: ...
:rtype: ...
|
|
||
| def get_duration_from_two_dates(date1: cftime.datetime, date2: cftime.datetime) -> str: | ||
| """ | ||
| Accept two dates and output duration in ISO8601. |
There was a problem hiding this comment.
same documentation comment
There was a problem hiding this comment.
also including any errors raised if I remember correctly
There was a problem hiding this comment.
this doc comment applies throughout the file
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