Skip to content

Conversation

@lionel42
Copy link
Collaborator

@lionel42 lionel42 commented Dec 8, 2025

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors code organization and enhances functionality by moving the format_plot_regions function to a more appropriate module, expanding unit detection capabilities, and adding improved handling for optional variables in flux data processing.

  • Moved format_plot_regions from flux_timeseries.py to regions.py for better code organization
  • Enhanced get_posterior_unit (renamed to get_unit) to support additional variable types including prior variables
  • Added conditional checks for optional posterior variables and improved error handling for missing data_dir when plotting inventory data

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File Description
tests/utils/test_format_region.py New test file for the relocated format_plot_regions function with tests for default behavior, error cases, and dataset extraction
tests/test_plots.py Added tests for plot_country_flux including default behavior and validation that data_dir is required when plotting inventory
fluxy/plots/flux_timeseries.py Updated imports, renamed and enhanced get_unit function, improved axes handling, added data_dir validation, and conditional posterior plotting
fluxy/operators/regions.py Added format_plot_regions function from flux_timeseries module, enhanced _extract_region_flux_sector to handle optional prior/posterior variables, improved Path handling
Comments suppressed due to low confidence (3)

fluxy/operators/regions.py:406

  • If ds_all is an empty dictionary, set.intersection with no arguments will raise a TypeError. Consider adding a check for empty ds_all or handling this edge case explicitly.
    fluxy/operators/regions.py:203
  • Extra whitespace after variable: there are two spaces between variable and =. Should be variable = var_of_step[v] with a single space.
    fluxy/operators/regions.py:156
  • If flux_{sector}_prior_country is not present in the dataset (skipped at lines 151-152), the subsequent sigma calculation code (lines 157-178) will raise a KeyError when trying to access this variable. Consider checking for the variable's existence before attempting to compute sigma values, or only compute sigma for variables that were successfully processed.
                        (
                            ds_region[f"flux_{sector}_prior_country"]
                            - ds_region[f"percentile_flux_{sector}_prior_country"].isel(
                                percentile=min_percentile_index
                            )
                        )
                        ** 2
                    ).sum(dim="country")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lionel42 lionel42 requested a review from melodb December 8, 2025 11:55
@lionel42
Copy link
Collaborator Author

lionel42 commented Dec 8, 2025

@melodb I had to fix few things to make some of my code work. Please check and let me know

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