Skip to content

Conversation

@eoydvin
Copy link
Collaborator

@eoydvin eoydvin commented Dec 16, 2025

W.I.P

Closes #58 and #75.

This PR updates the notebooks according to suggestions in #58. It also adds a introduction notebook where we show more detailed how mergeplg works.

@cchwala
Copy link
Member

cchwala commented Dec 16, 2025

Big thanks for cleaning up and polishing the notebooks! 👏 👏 👏

I only had a brief look. Things look already pretty good. The only thing I would definitely change is the colormap. Not because I do not like viridis, but in the dark regions one cannot see the gauge and CML locations. Also the contrast of viridis is not optimal. With turbo more details are visible. But of course, the choice of colormap is a bit personal taste...

@codecov
Copy link

codecov bot commented Jan 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.37%. Comparing base (0d5f302) to head (6ad9dab).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage   95.37%   95.37%           
=======================================
  Files          11       11           
  Lines         800      800           
=======================================
  Hits          763      763           
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eoydvin
Copy link
Collaborator Author

eoydvin commented Jan 8, 2026

@cchwala I find the poligrain plotting functions very nice, but for plotting several figures in one row with one shared colorbar using plt.subplots I have do do some workarounds to make it look nice. Do you think the code for doing this is ok as it is now? I also have to use mm/h as unit since poligrain uses the closest 10-values when setting hexbin extents. Its ok by me, but I had to look at the source code to understand the behavior.

I also changed the colormap, better now?

The introduction notebook could maybe need some short review.

Note that I will keep working on these notebooks while doing parallel work on the OpenMRG2 notebooks. So should we wait with merging?

@cchwala
Copy link
Member

cchwala commented Jan 8, 2026

...but for plotting several figures in one row with one shared colorbar using plt.subplots I have do do some workarounds to make it look nice. Do you think the code for doing this is ok as it is now?

I assume you mean the part of plotting the maps with im = ax[i].pcolormesh where you need im for the colorbar, correct? This looks good to me. We could improve the poligrain plotting functions e.g. to return the "artist collection" (or whatever matplotlib calls this) so that it can be used when adding a colorbar manually. But maybe later... In the case of plot_plg it is also not clear which of the three or all of the plotted objects should be returned.

I also have to use mm/h as unit since poligrain uses the closest 10-values when setting hexbin extents.

The hexbin plotting function from poligrain is indeed a bit restrictive and not very adjustable.

The introduction notebook could maybe need some short review.

I just had a brief look because I am too busy now to go into the details. Overall it looks good, maybe a bit long for an introduction. You can leave it as it is since it gives a nice overview. If you want to spend time on this, you could rename it to "overview" and start a very short notebook that just shows the basic concept for doing the merging using only one method, just to make clear how things are applied. But this is not a priority now IMO.

Note that I will keep working on these notebooks while doing parallel work on the OpenMRG2 notebooks. So should we wait with merging?

Yes, let's wait with merging.

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.

Update notebooks

2 participants