search

MBravoS / splotch

Simple PLOTs, Contours and Histograms is a small package with wrapper functions designed to simplify plotting calls from matplotlib.
BSD 3-Clause "New" or "Revised" License
4 stars 0 forks source link

grid=None behaviour in plot_finalizer #66

Closed AstroRobin closed 1 year ago

AstroRobin commented 2 years ago

Currently, if grid=None in any plot call, the plot_finalizer will look to the rcParams for the default grid choice. However, this creates a strange behaviour if one sets grid=True in a one function then leaves it out (or sets grid=None explicitly) in a subsequent splotch function. The function defaults to the rcParams, irrespective of whether the current axis already has grid turned on.

Instead, I think plot_finalizer should first check the current status of the grid before defaulting back to the rcParams. If off: use default rcParams. If on: it has been turned on somewhere in the code, so leave it be.

MBravoS commented 2 years ago

Instead, I think plot_finalizer should first check the current status of the grid before defaulting back to the rcParams. If off: use default rcParams. If on: it has been turned on somewhere in the code, so leave it be.

That would still lead to issues. Example: you have grids turned on by default in rcParams, make a first plot with grid=false and a second one without passing anything to grid, that would turn the grid back on. I think the better approach is:

If an axis exist (before making the plot call) or if given through ax:     Use the parameter given to grid, if None use the rcParam value Else:     Use the parameter given to grid, if None do nothing

AstroRobin commented 2 years ago

Okay, yes I see where my solution might also go wrong.Okay, so it's a check as to whether ax is not None and plt.gcf().get_axes() == []. The reason you can't use plt.gca() is that if no axis exists, matplotlib will create one for you.

MBravoS commented 1 year ago

Thinking about this issue again, I wonder if is not best to remove the grid functionality from splotch. The reason why is this an issue at all is because we handle the grid with every plot call, which is not how it is handled in matplotlib. While is a nice functionality, I don't see how we could fix this issue while keeping the grid call on plot_finalizer. What do you think @AstroRobin?

AstroRobin commented 1 year ago

Could we parse grid=None (as default), which does not change the current grid status from what it is? Perhaps this requires a check to see if there already are grids plotted or not. Not sure how this would work with rcParams, however.

AstroRobin commented 1 year ago

I've made an implementation of the new grid functionality in https://github.com/MBravoS/splotch/commit/9d8129cb14609fb39b8e7473f8f9e14498bd6a87. It uses a new base function called grid_handler() to decide how to receive the various possible inputs to grid, which can be one of:

The dictionary input is actually rather useful as it means you can directly modify grid properties from any plot call, but it does not change the functionality of the boolean usage of grid.

Making this possible also required a slight rework of the plot_finalizer() function, specifically to apply properties to a provided axis argument rather than to the current axis. For now, this reworked version is named _plot_finalizer() just to be safe. Currently, all plot functions point to this new version, which should replicate the previous behaviour, but let's test it before we transition completely.

I also standardised the way we have been assigning the old_ax in our functions.

AstroRobin commented 1 year ago

This seems to be working so far in devel, so I'm closing this issue for now.

MBravoS commented 1 year ago

It seems to work, and adding a couple of parameters as defaults was really nice*, but isn't the choice of dashed lines as the default choice for grids... perhaps controversial? matplotlib's default choice for this is solid lines, after all.

*We really need to have a look across all other functions and see what else could be helpful to have in the default parameters (#30).

AstroRobin commented 1 year ago

Indeed that is controversial and most likely a remnant of my testing of the defaults code. Fixed in https://github.com/MBravoS/splotch/commit/f5047832189db828d75e6b3887807fecaa46da0c

I agree, #30 should be our next big focus.

MBravoS commented 1 year ago

Neat, I also folded the new defaults into our default style sheet and released the new version.