Conversation
- required by fillna_method of 'nearest'.
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to handle NaN (Not a Number) values in the data retrieval process by providing configurable fill methods. Previously, the code was hardcoded to use forward fill ('ffill') for handling missing data.
- Adds a new
--fillna-methodcommand line argument with multiple options for handling NaN values - Implements support for interpolation methods ('nearest', 'linear') that require scipy as an optional dependency
- Updates the data processing logic to conditionally apply different fill strategies based on user preference
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| main/scripts/get.py | Adds command line argument for fillna method and passes it to data processing function |
| main/contrib/data.py | Implements the core NaN handling logic with multiple fill methods and scipy dependency checking |
| main/init.py | Adds scipy installation detection similar to existing tqdm detection pattern |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
main/scripts/get.py
Outdated
| help="Print the site configuration with essential dependencies and their versions.") | ||
| parser.add_argument('--fillna-method', dest='fillna_method', default='ffill', | ||
| help="The method defines how the invalid data (NaN) should be filled, " | ||
| "defaults 'ffill', the last valid one is used, other options: " |
There was a problem hiding this comment.
There's a grammatical error in the help text. 'defaults 'ffill'' should be 'defaults to 'ffill''.
| "defaults 'ffill', the last valid one is used, other options: " | |
| "defaults to 'ffill', the last valid one is used, other options: " |
main/contrib/data.py
Outdated
| if fillna_method in ("nearest", "linear"): | ||
| data.interpolate(method=fillna_method, inplace=True) | ||
| if fillna_method == "ffill": | ||
| data = data.bfill() |
There was a problem hiding this comment.
There's a logic error: both 'ffill' and 'bfill' methods call data.bfill(). The 'ffill' case should call data.ffill() instead.
| data = data.bfill() | |
| data = data.ffill() |
main/contrib/data.py
Outdated
| if fillna_method == "nearest" and not SCIPY_INSTALLED: | ||
| _LOGGER.warning("Install scipy to support fillna_method 'nearest', fallback to 'ffill'.") | ||
| fillna_method = "ffill" | ||
| if fillna_method in ("nearest", "linear"): | ||
| data.interpolate(method=fillna_method, inplace=True) | ||
| if fillna_method == "ffill": | ||
| data = data.bfill() | ||
| if fillna_method == "bfill": | ||
| data = data.bfill() |
There was a problem hiding this comment.
The 'none' option mentioned in the help text is not handled in the implementation. Consider adding an explicit check for 'none' to make the code more maintainable and clear about when no filling should occur.
| if fillna_method == "nearest" and not SCIPY_INSTALLED: | |
| _LOGGER.warning("Install scipy to support fillna_method 'nearest', fallback to 'ffill'.") | |
| fillna_method = "ffill" | |
| if fillna_method in ("nearest", "linear"): | |
| data.interpolate(method=fillna_method, inplace=True) | |
| if fillna_method == "ffill": | |
| data = data.bfill() | |
| if fillna_method == "bfill": | |
| data = data.bfill() | |
| if fillna_method == "none": | |
| _LOGGER.info("No filling of missing data will be performed (fillna_method='none').") | |
| else: | |
| if fillna_method == "nearest" and not SCIPY_INSTALLED: | |
| _LOGGER.warning("Install scipy to support fillna_method 'nearest', fallback to 'ffill'.") | |
| fillna_method = "ffill" | |
| if fillna_method in ("nearest", "linear"): | |
| data.interpolate(method=fillna_method, inplace=True) | |
| if fillna_method == "ffill": | |
| data = data.bfill() | |
| if fillna_method == "bfill": | |
| data = data.bfill() |
No description provided.