Skip to content

Fix kerchunk JSON file filtering#54

Merged
honzaflash merged 3 commits into
mainfrom
fix-kerchunk-files
Nov 19, 2025
Merged

Fix kerchunk JSON file filtering#54
honzaflash merged 3 commits into
mainfrom
fix-kerchunk-files

Conversation

@honzaflash

@honzaflash honzaflash commented May 30, 2025

Copy link
Copy Markdown
Contributor

Changes

  • refactor to deduplicate code
    • the JSON file filtering gets its own function
  • simplify how dates are passed around for the kerchunk functions
  • and this way fix the date format mismatch when filtering the JSON files
    • start/end dates are kept as datetime and dates from the filenames get converted to it to compare

The issue before:

  • make_ciofs_kerchunk would receive dates as string formatted like YYYY_0DDD or YYYY-MM-DD
  • then parse the date from the filename like this: datetime.strptime(Path(j).stem, date_format).isoformat() getting YYYY-MM-DDT...
  • for CIOFS this would lead to comparison like this '2022_0213' <= '2022-09-01' which is false
    • even though '2022_0213' == datetime(2022,8,1).strftime('%Y_0%j') and 2022-08-01 is before 2022-09-01
    • because '_' <= '-' is false

The fixed issue after:

  • make_ciofs_kerchunk now receives start/end dates as datetime objects
  • filenames are parsed into datetime objects as well for the comparison
  • if future models require more flexibility in how the date time is represented in the file names the base_str could always be refactored to also accept a datetime object instead of the year 'YYYY' string

Testing

  • the project test suite is passing
  • seeding a CIOFS simulation now succeeds

@honzaflash

Copy link
Copy Markdown
Contributor Author

I hope I didn't miss some reason why the start/end dates were the way they were.

Also let me know if the changes in whats_new.md are what you were imagining.

@kthyng

kthyng commented May 30, 2025

Copy link
Copy Markdown
Collaborator

Ok I see what you are doing here. I remember working on this but clearly didn't leave it working correctly!

Can you add a test to demonstrate this working correctly? Be sure to include the edge case of starting a test right at the end of the year that will need simulation output from both that year and the following year (ideally it would work if the simulation was running backward in time too, though I haven't been testing that behavior yet).

- refactor to deduplicate code
- simplify how dates are passed around for the kerchunk functions
- this was originally done to fix the date format mismatch when filtering
  the JSON files but this bug got fixed while this PR was waiting for review
  so it remains as mostly just a refactor after rebasing and resolving
  conflicts
@honzaflash

Copy link
Copy Markdown
Contributor Author

Rebased onto main and resolved conflicts.

Since the bug was fixed in main, this is now just more of a refactor and adding of tests.

@kthyng

kthyng commented Nov 19, 2025

Copy link
Copy Markdown
Collaborator

@honzaflash Ok I took a look and also added a comment to whats new since I forgot for the last PR. Looks good!

@honzaflash
honzaflash merged commit 80bd630 into main Nov 19, 2025
5 checks passed
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