Skip to content
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

Fix comparison plot creation logic #87

Merged

Conversation

nfb2021
Copy link
Collaborator

@nfb2021 nfb2021 commented Oct 17, 2024

A preparation for stability metrics & improvement of intra-annual metrics

The logic used to create the clustered boxplots to compare a given metric for all temporal sub-windows present was changed.

Before, each temporal sub-window box in the boxplot could be calculated over potentially different GPIs. This would happen if the test area was masked during certain months or seasons, e.g. for snow or frozen soil. As as result, a direct comparison of these boxes was difficult.

Now, a spatial subset is chosen so that the same GPIs with valid data (=based on a predefined threshold for amount of non-NaNs) are used for all temporal sub-windows present. The exception are temporal sub-windows that have so many NaNs, that they are above the threshold. These temporal sub-windows are then entirely set to Nan and no box is plotted for them, even though the temporal sub-window names will appear in the final plot.

This ensures a more robust comparability between different temporal sub-windows for a given metric.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11388336978

Details

  • 36 of 37 (97.3%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 81.405%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/qa4sm_reader/plotter.py 32 33 96.97%
Totals Coverage Status
Change from base Build 11274863906: 0.2%
Covered Lines: 2386
Relevant Lines: 2801

💛 - Coveralls

@wpreimes
Copy link
Collaborator

wpreimes commented Oct 18, 2024

you can merge this from my side if you think this is the correct approach. Generally, I wouldn't cherry-pick the results for the plots, because it makes it difficult to interpret the results. Plots should just visualize the results as they are in the netcdf file, people should be able to make their own versions of the plots from the results. If the coverage differs over the year, that should also be reflected in the plots (that's why we report the number of points with the boxes usually and have a separate nobs boxplot). For stability I guess it could make sense, in order to compute a meaningful slope, but then it probably should already be done as part of the metric calculation, rather than the visualisation.

@nfb2021 nfb2021 merged commit 650946f into awst-austria:master Oct 18, 2024
5 checks passed
@nfb2021
Copy link
Collaborator Author

nfb2021 commented Oct 18, 2024

you can merge this from my side if you think this is the correct approach. Generally, I wouldn't cherry-pick the results for the plots, because it makes it difficult to interpret the results. Plots should just visualize the results as they are in the netcdf file, people should be able to make their own versions of the plots from the results. If the coverage differs over the year, that should also be reflected in the plots (that's why we report the number of points with the boxes usually and have a separate nobs boxplot). For stability I guess it could make sense, in order to compute a meaningful slope, but then it probably should already be done as part of the metric calculation, rather than the visualisation.

I get your point and partially agree. It does facilitate a direct comparison though, if it is guaranteed, that always the same GPIs are used. Even though good data from other GPIs for some temporal sub-windows will be disregarded.

This approach will be revisited for the stability metrics, though, anyhow.

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.

3 participants