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

Plotting updates #129

Merged

Conversation

eleanorfrajka
Copy link
Collaborator

Ok - I fiddled around a little to change the aspect ratio in plots (main change). There may be a more elegant way to do this, but it was because the mplstyle only has one figure size option. I would like a way to have full width and half width plots or otherwise a little more overall control on the figure size generated.

Hence - two more variables at the top of plots.py for the widths, and a couple figures where I then used the width to generate within the figure plot) an appropriate height of the figure.

Also a couple label changes.

-- I'll need to fix the tests.

@eleanorfrajka
Copy link
Collaborator Author

Ok - one change I made in here, which isn't nice, is a selection of the plot width (full_width or half_width) and then scaling the figure height based on the width (since what is plotted looks best with a specified aspect ratio).

Chiara suggested a better way to do this would be to have a single figsize in the mplstyle sheet, and then a boolean within the plot to decide whether to make it full width or half width. But, I can't figure out how to nicely get values out of the style sheet. It looks like yaml, so could read as a yaml, but this would mean needing to import another package.

Or - maybe just allow the figure to generate with the default size, and then pull out the figure width and rescale within the plot?

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -17,6 +17,37 @@
dir = os.path.dirname(os.path.realpath(__file__))
glidertest_style_file = f"{dir}/glidertest.mplstyle"

def _time_axis_formatter(ax, ds, format_x_axis=True):
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should go on utilities instead

@@ -38,6 +69,8 @@ def plot_updown_bias(df: pd.DataFrame, ax: plt.Axes = None, xlabel='Temperature
with plt.style.context(glidertest_style_file):
if ax is None:
fig, ax = plt.subplots()
third_width = fig.get_size_inches()[0] / 3.11
Copy link
Member

Choose a reason for hiding this comment

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

I think this is good! Do we want to have the option to have the third/half/full in the parameters instead and put third as default here.
Something like:

def plot_updown_bias(df: pd.DataFrame, ax: plt.Axes = None, xlabel='Temperature [C]', size='third', '**kw: dict, ) -> tuple({plt.Figure, plt.Axes}):
if size=='third':
     third_width = fig.get_size_inches()[0] / 3.11
     fig.set_size_inches(third_width, third_width *1.1)
...

Or if we want 3 styles and the font size is also adjusted

@callumrollo
Copy link
Member

Love the conditional time axis formatter! I agree with Chiara that it probably belongs in utilities

With regards to setting user-defined figure sizes, don't stress too much about it. One of the reasons we return fig, ax is so that the user can e.g. resize the plot afterwards to match whatever style they want e.g. with figure.set_size_inches

This gives lots of flexibility, without needing to add a bunch of optional arguments to each funtion

image

This is something we should demonstrate in the notebook I think!

Copy link
Member

@callumrollo callumrollo left a comment

Choose a reason for hiding this comment

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

looks good to me!

@eleanorfrajka eleanorfrajka merged commit 3405cbe into OceanGlidersCommunity:main Nov 20, 2024
10 checks passed
@eleanorfrajka eleanorfrajka deleted the eleanor-reverted-11 branch November 20, 2024 11:57
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