Skip to content

Conversation

@hoyer-a
Copy link
Member

@hoyer-a hoyer-a commented Dec 11, 2025

Note

  • Pyfar plotstyle did not really improve the colorbar-placement for balloon plots that extend in y-direction. I would suggest to update the data used in the examples in the docs so it extends in x-direction in a separate pull.
  • Test are failing because of Failing Tests Scipy 1.17 #261

Merges into #222

Changes proposed in this pull request:

@hoyer-a hoyer-a added this to the v1.0.0 milestone Dec 11, 2025
@hoyer-a hoyer-a added the plot label Dec 11, 2025
@hoyer-a hoyer-a moved this from Backlog to Require review in Weekly Planning Dec 11, 2025
@github-project-automation github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Dec 11, 2025
Copy link
Member

@f-brinkmann f-brinkmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lots of plots where color indicated amplitude before and now indicates phase. Is that desired?

@hoyer-a
Copy link
Member Author

hoyer-a commented Dec 11, 2025

The default behavior changed when we replaced phase=False/True with cmap_encoding=phase/magnitude, but apparently many baseline figures were never updated. We briefly spoke about this in the last weekly.

Related pull #184

@sikersten
Copy link
Member

Thanks for taking care! Some of the baseline plots have not changed. Should we only include those that changed to keep a clean history?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just using this as an example, the same applies for some of the plots below:
The colorbar obstructs the tick and axis labels. Wasn't this improved as part of #220?

Copy link
Member Author

@hoyer-a hoyer-a Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed solution was to return axes and colorbar and allow lists of axes, so the plot / colorbar layout can be adjusted individually
#211 (comment)

Copy link
Member

@f-brinkmann f-brinkmann Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there is a simple fix for this, like increasing the figure size? If yes, this would be fine for me. If not it might be worth improving the default behaviour - but probably in a separate pull.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment to the original issue on this #183
Maybe let's explore this first before finalizing the new baseline plots.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I will look into this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should fix this in an other PR, because this PR is just to update the baseline, in case were we forgot it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anton and I found that this remains a Matplotlib problem for which there is no universal default solution. As discussed in the weekly, people will have to specify their own axis in some cases to avoid overlaps. Anton will suggest a docstring for the plot-module which explains this and gives an example on how avoid it using grid specs

@hoyer-a hoyer-a force-pushed the plots/create-baseline-plots branch from 42df3ff to 1d58eed Compare December 15, 2025 11:19
@hoyer-a hoyer-a requested review from f-brinkmann and mberz December 15, 2025 11:19
@hoyer-a hoyer-a moved this from Reviewer Approved to Require review in Weekly Planning Dec 15, 2025
@mberz mberz moved this from Require review to On hold in Weekly Planning Dec 15, 2025
@hoyer-a hoyer-a force-pushed the refactor/collect_cb_changes branch from 8311b1e to cd4c63a Compare January 9, 2026 11:02
@hoyer-a hoyer-a force-pushed the plots/create-baseline-plots branch from 1d58eed to bfd5d5c Compare January 9, 2026 12:38
@mberz
Copy link
Member

mberz commented Jan 9, 2026

Does this already include #257?
If so, please move it to "require review" again.

@hoyer-a hoyer-a requested review from a team and ahms5 January 13, 2026 08:53
@hoyer-a hoyer-a moved this from On hold to Require review in Weekly Planning Jan 13, 2026
@hoyer-a
Copy link
Member Author

hoyer-a commented Jan 13, 2026

Now the baseline plots include #257. Unfortunately the colorbar-placement did not improve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anton and I found that this remains a Matplotlib problem for which there is no universal default solution. As discussed in the weekly, people will have to specify their own axis in some cases to avoid overlaps. Anton will suggest a docstring for the plot-module which explains this and gives an example on how avoid it using grid specs

@github-project-automation github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Reviewer Approved

Development

Successfully merging this pull request may close these issues.

6 participants