-
Notifications
You must be signed in to change notification settings - Fork 3
Add windrose for eddy flux #219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 implements windrose plotting functionality for eddy flux data by extending the existing stacked plot functionality to support wind direction grouping. It adapts the plot_stacked function to create polar coordinate plots when wind direction data is available.
- Adds windrose plotting capability to the eddy flux module
- Extends the stack_plot function to support both area and bar plots with configurable width
- Updates configuration files with new sector groupings and color mappings
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| fluxy/plots/ec_flux/sectorial_stack.py | Main implementation of windrose functionality with polar coordinate plotting |
| fluxy/plots/utils.py | Enhanced stack_plot function to support both area and bar charts with width parameter |
| tests/ecflux/test_ecflux_operators.py | Added test case for windrose plotting functionality |
| scripts/example_ecflux.ipynb | Documentation examples showing windrose usage |
| configs/sectors.yml | Updated sector groupings and color mappings |
Comments suppressed due to low confidence (2)
fluxy/plots/ec_flux/sectorial_stack.py:1
- Corrected spelling of 'beaviour' to 'behaviour'.
import xarray as xr
fluxy/plots/ec_flux/sectorial_stack.py:1
- Corrected spelling of 'paramters' to 'parameters'.
import xarray as xr
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
melodb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested the example notebook and all is working fine. However, would you like to reconsider your bins splits? In my view, a wind rose with 4 bins should have 4 slices centered around N-E-S-W. That is, between 315 deg and 45 deg and so on. Currently, your slices are centered around NE-SE-SW-NW, which in my view is not standard.
|
Just to remind you about this PR. Are you planning to work on it soon? |
Currently I am a busy with other things, but I will try to finish it before christmas ;) |
thanks for warning this, I adapted to follow the standard notation in the last commit. |
|
@melodb thanks for the feedback, I am done with the corrections, ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (3)
fluxy/plots/utils.py:956
- The 'colors_of_category' parameter is not documented in the docstring. This parameter should be documented to explain its purpose and expected format.
colors_of_category: dict[str, str] = {},
width: float = 0.8,
):
"""Function to plot stacked bar plots for the emissions data.
Parameters
----------
df : pd.DataFrame
DataFrame containing the emissions data.
ax : matplotlib.axes.Axes, optional
Axes object to plot on, by default None
area : bool, optional
If True, use area plot instead of bar plot, by default False
"""
fluxy/plots/ec_flux/sectorial_stack.py:225
- When wind_plot is True and area is True, df_sim is modified to add an extra row to close the circle (line 197-198), but df_obs is not modified. This means that when ax.scatter is called at line 219-225, df_sim.sum(axis=1) will have one more element than df_obs.index, potentially causing a plotting mismatch or error. The same issue affects the errorbar call at lines 213-217. Consider either: (1) filtering df_sim back to the original indices before plotting observations, or (2) also appending the duplicated observation data to df_obs.
if wind_plot and area:
# Add extra row, to close the circle
old_index = df_sim.index
df_sim = pd.concat([df_sim, df_sim.iloc[[0]]])
df_sim.index = np.append(old_index, 2 * np.pi + old_index[0])
ax = stack_plot(
df_sim,
ax=ax,
area=area,
colors_of_category=sectors_config.get("colors_of_sector", {}),
width=0.8 if not wind_plot else (wind_bins[1] - wind_bins[0]),
)
if "yerr" not in errorbar_kwargs:
errorbar_kwargs = errorbar_kwargs.copy()
yerr = df_obs["std"].values.reshape(-1)
yerr[np.isnan(yerr)] = 0.0
errorbar_kwargs["yerr"] = np.array(yerr)
ax.errorbar(
df_obs.index,
df_obs["mean"].values.reshape(-1),
**errorbar_kwargs,
)
# scatter the total simulated
ax.scatter(
df_sim.index,
df_sim.sum(axis=1),
color="black",
marker="*",
label="Total simulated fluxes",
)
fluxy/plots/utils.py:956
- The newly added 'width' parameter is not documented in the docstring. The docstring should include documentation for this parameter to maintain consistency and help users understand its purpose.
width: float = 0.8,
):
"""Function to plot stacked bar plots for the emissions data.
Parameters
----------
df : pd.DataFrame
DataFrame containing the emissions data.
ax : matplotlib.axes.Axes, optional
Axes object to plot on, by default None
area : bool, optional
If True, use area plot instead of bar plot, by default False
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
melodb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All is working fine. Feel free to merge. The only thing that does not look beautiful is the number of counts in the wind rose, which sometimes overlaps with the measurement uncertainty.
For a next PR then ;) |
Addapting the stack plot and using the winddirection data, this PR creates nice windrose plots that can show the eddy flux signal based on the wind direction.