Add mf timeseries for multiple sites

[alexdanjou]

I allows to plot multiple sites on the same graph. It is I think interesting to look at and compare the mf seasonnal cycle between some choosen sites.

See example (not choosen randomly 😅):

[melodb]

Didn't @hdelongueville work on the same thing?

[hdelongueville]

I think here it is on the same plot while "mine" is on several subplot.

[hdelongueville]

I'll look at it when I'm creating a PR for mine :)

[hdelongueville]

I had a quick look at this PR and it seems that it is not "just adding a new mf function" but is also changing some of flux_timeseries.. which suggests that it is not a straightforward PR. Because I don't have a lot of time, I'll let you decide what you want to do with it, but looks like a nice feature to have in Fluxy!

[mo-aliceramsden]

It might be worth having a quick chat about this at the next PARIS meeting (when we have time) to decide what format we want these plots in, then we can decide which of the two PRs on this to merge in to develop?

[melodb]

I took a quick look at the code and it seems that there is a new function add_line_plot which is now used to plot both the mf timeseries and the flux timeseries. We might want to check the pros and cons of this option. If we keep it, we might want to move this function to other location (maybe mf_timeseries.py is not the best one if it is a generic function). The approach followed here seems a bit different than the approach followed in #266. We might want to uniformize the code.

I tested the notebook and all plots are working fine. I have added a cell with a call to the new function plot_sites_list_mf to facilitate testing. After my commit, the tests are failling. Honestly, I don't know why they were passing before and are failling now. I have simply added a cell in the notebook.

[moPeterAndrews]

Once I updated all the scripts it worked without issue, so not sure what's happening with yours Daniela.

Comparing to #266, this is probably better for comparing sites, as sites are on the same graph, and each graph is for a model, whereas #266 is a different axis for each site. Not sure which approach is better, though this seems to be a bit faster, and the seasonal cycle plotting works for this version where it doesn't on #266

[melodb]

Once I updated all the scripts it worked without issue, so not sure what's happening with yours Daniela.

Comparing to #266, this is probably better for comparing sites, as sites are on the same graph, and each graph is for a model, whereas #266 is a different axis for each site. Not sure which approach is better, though this seems to be a bit faster, and the seasonal cycle plotting works for this version where it doesn't on #266

Thanks, Peter, for checking this! There is no problem with my version. It's just the GitHub tests which are failling (see message "All checks have failed" close to the red cross).

Regarding the comparison with #266, I think both plots are usefull: 1) one subplot per model with all sites vs 2) one subplot per site with all models. It depends on the analysis you are performing. We can try to move along with this PR, but add option 2 as well.

[melodb]

@moPeterAndrews: I think it fails simply because devel is failling. The error message points to:

E File "/home/runner/work/fluxy/fluxy/fluxy/operators/flux_map_resample.py", line 274
E f"{first_time.strftime([[1:3], [7:9]])}"
E ^
E SyntaxError: f-string: expecting '=', or '!', or ':', or '}'

which was introduced 3 merges ago. This will be fixed soon.

[moPeterAndrews]

I just added the ability to switch between plotting sites and models on separate graphs - the only change is in plot_sites_list_mf, it just switches which order it loops over sites and models.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.

Comments suppressed due to low confidence (1)

fluxy/plots/mf_timeseries.py:28

• Duplicate import: set_min_decimal_points is imported twice. Remove one of the imports to avoid confusion and keep the module header clean.

from fluxy import config
from fluxy.plots.utils import set_min_decimal_points
from fluxy.types import VariableType
from fluxy.operators.select import (
    FrequencyType,
    clean_timeseries_missing_data,
    get_site_index,
    get_unique_site_height_pairs,
    slice_site,
    check_site_list,
)
from fluxy.plots.utils import set_min_decimal_points

---

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

[lionel42]

@melodb @moPeterAndrews I went to some more changes after merging with the main branch.

I also added the new function in the example notebook, and fixed it for what was needed.

Can you test your main notebooks if they still work as intended ?

[melodb]

I will be on sick leave until the end of the day so @mo-aliceramsden and @moPeterAndrews I will leave the testing for you if that's ok. I left only a comment from the revision I started yesterday.

[moPeterAndrews]

Any plotting of timeseries has incorrect uncertainties, they're far bigger here than in the "devel" branch. I'd suggest reverting commit #a64b379, as this will bring it back into alignment with the "devel" branch.

It looks like this commit was made to resolve the discrepancy between "FillBetween" and "Errorbar" uncertainty styles, but the wrong one was corrected. A better fix might be:

` mean = da.sel(percentile="mean").values

if plot_type == "FillBetween":
    ax.fill_between(
        time_as_datetime,
        y1=min_unc,
        y2=max_unc,
        alpha=0.2,
        color=da.attrs["plot_color"],
    )

elif plot_type == "Errorbar":
    ax.errorbar(
        time_as_datetime,
        y=da.sel(percentile="mean"),
        yerr=[mean-min_unc, max_unc-mean],
        alpha=0.4,
        fmt="none",
        color=da.attrs["plot_color"],
    )`

[lionel42]

@mo-aliceramsden Have you had time to test this branch ? Can we merge it ?

[mo-aliceramsden]

I've tested out the new plotting option and this is all working well for me. I'm happy for this to be merged in to devel