Preserve FCI time units from source files - #3428
Open
sapunyangkut wants to merge 1 commit into
Open
Conversation
Remove static time-unit overrides so the FCI reader retains complete CF reference-time units from source files, with regression coverage for all time datasets. Developed and checked with OpenAI Codex assistance.
Member
|
Hi @sapunyangkut , thanks for this! Is there a reason for the PR to be still in draft? It looks quite complete to me. We usually mark the PR as Ready for Review when the work is done and the PR is potentially ready to be merged. |
ameraner
added
enhancement
sapunyangkut
marked this pull request as ready for review
August 23, 2026 12:01
sapunyangkut
requested review from
ameraner,
djhoese,
gerritholl and
mraspaud
as code owners
August 23, 2026 12:01
djhoese
reviewed
Aug 24, 2026
Comment on lines
+848
to
+852
| for data_id, info in time_datasets: | ||
| loaded_time = source_time.copy() | ||
| loaded_time.attrs = file_handler._set_and_cleanup_attributes( | ||
| loaded_time.attrs, info=info) | ||
| assert loaded_time.attrs["units"] == expected_units |
Member
There was a problem hiding this comment.
Is it possible to call the higher-level file handler functions to get these datasets out and compare the .attrs["units"] and the actual values? This way if the code is every refactored this test would still pass (not calling a private method)?
Additionally, is the reflectance units check expected below? Or copy/paste from before?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
remove static
soverrides from FCI time-dataset configurationpreserve complete CF reference-time units supplied by the source NetCDF file
cover every generated FCI time dataset while confirming non-time units remain unchanged
Closes FCI L1C reader overwrites units attribute on time metadata, making the data hard to interpret #3349
Tests added
Author entry is already included in open Draft Ignore None dataset timestamps in Scene bounds #3427; no duplicate
AUTHORS.mdchange is made hereValidation
git diff --checkpassedgit diff --check, Python byte-compilation of the changed test, and non-constructing YAML syntax parsing passedThe current checkout is not installed as a package, so a fresh pytest collection stops at the repository's generated
satpy.versionimport;ruffis also unavailable in this environment. No dependency was installed.AI assistance disclosure
This patch, tests, commit message, and pull request text were developed and checked with OpenAI Codex assistance. No independent human self-review is claimed; maintainer review is requested before merge.