contains hotfixes for plotting utilities #133
Conversation
- fixed issue where border was drawn incorrectly - supressed std out message if unable to obtain region_border - axes can be chained for plot_consistency_test_result
- grab xticks from axes correly in plot_comparison_test - properly adjust xlim in plot_comparison_test
Codecov Report
@@ Coverage Diff @@
## master #133 +/- ##
==========================================
- Coverage 57.06% 56.97% -0.10%
==========================================
Files 19 19
Lines 3142 3147 +5
Branches 454 455 +1
==========================================
Hits 1793 1793
- Misses 1238 1243 +5
Partials 111 111
Continue to review full report at Codecov.
|
csep/core/regions.py
Outdated
| desc.append(poly_max[2]) | ||
| desc.append(poly_max[3]) |
There was a problem hiding this comment.
This change loooks good, but made me realize a detail, that the polygons defined in the CSEP built-in italy region is not 100% correct
See the serrated edges, which does not happens if the Catalog region is assigned to the one that is imported from a forecast like this:
There was a problem hiding this comment.
i think this issue is from the way i am pulling out the outermost region. thanks for spotting this, i'll come up with a solution so it works for both cases.
There was a problem hiding this comment.
i pushed a hotfix for this issue. i checked it against the built in region. everything looks to work properly. if you have this pr cloned do you mind checking as well?
csep/utils/plots.py
Outdated
| ax.set_ylim([ylims[0], ylims[1]]) | ||
| ax.set_xlim([ax.get_xlim()[0] + 0.5, ax.get_xlim()[1] - 0.5]) | ||
| ax.set_ylim([ylim[0], ylim[1]]) | ||
| ax.set_xlim([ax.get_xlim()[0] - 0.5, ax.get_xlim()[1] + 0.5]) |
There was a problem hiding this comment.
This ax.set_xlim() change gives an extra edge border that I'm not sure is needed. This gives
versus this, which was in the previous xlim definition
But this failed for low number of Models in the t-test. I'd suggest to leave it better like:
ax.set_xlim([-0.5, len(results_t) - 0.5])
It appears to work well with any number of forecasts
There was a problem hiding this comment.
that doesn't seem to work with only two models, it only displays one
if i use
ax.set_xlim([-0.5, len(results_t) + 0.5])
it seems to work ok. am i missing something here?
There was a problem hiding this comment.
i really like the way these plots look!
There was a problem hiding this comment.
Weird! Using ax.set_xlim([-0.5, len(results_t) - 0.5]) from the cloned pycsep_esrl repo I got this plot correctly:
There was a problem hiding this comment.
i think i got it. i just pushed a new commit with that fix.
* try to determine which points to plot based on latitude values
* update xlim in plot_comparison_test
csep/utils/plots.py
Outdated
| @@ -1342,7 +1342,7 @@ def plot_comparison_test(results_t, results_w=None, axes=None, plot_args=None): | |||
| xTickPos = ax.get_xticks() | |||
| ax.yaxis.set_major_locator(matplotlib.ticker.MaxNLocator(integer=True)) | |||
| ax.set_ylim([ylim[0], ylim[1]]) | |||
| ax.set_xlim([ax.get_xlim()[0] - 0.5, ax.get_xlim()[1] + 0.5]) | |||
| ax.set_xlim([0.5, len(results_t) - 0.5]) | |||
There was a problem hiding this comment.
nope, this still hides me a forecast. It works for me with [ - 0.5, len(results_t) - 0.5]. Weiirdd.
There was a problem hiding this comment.
missed the '-'. catch you tomorrow! gn!
* update xlim in plot_comparison_test
@pabloitu do you mind taking a look at this pr when you have a chance?
contains a few adjustments to the plotting utilites