Adding function ctd_plot and solving conflicts

[FloraSauerbronn]

Following instructions from #110
New branch from upstream and add new ctd plot function.

[ocefpaf]

@FloraSauerbronn this is the image diff:

Again, it does seem to be related to fonts. Can you try to create a fresh environment with conda just to ensure you will get the same fonts we have in the CIs? We have some instructions on how install miniforge, a conda provider, and create environments in https://ioos.github.io/ioos_code_lab/content/ioos_installation_conda.html

Then, once you have conda, you can create an environment for gliderpy with the commands:

conda create --name GLIDERPY python=3 --file requirements.txt --file requirements-dev.txt
pip install -e . --no-deps --force-reinstall

With that said, if you check the logs, the RMS on the image diff is ~16.5. See https://github.com/ioos/gliderpy/actions/runs/9713713840/job/26811095367?pr=115#step:5:24.

That means you can add a tolerance of 17 for the test and get them to pass.

PS: Also check the pre-commit logs and see if you can sort them out.

[FloraSauerbronn]

@ocefpaf But I should change the tolerance even if using the Conda from IOOS work ?

[ocefpaf]

@ocefpaf But I should change the tolerance even if using the Conda from IOOS work ?

Did you try that on Windows and the diffs are still ~16.5? If so, yes.

[ocefpaf]

@FloraSauerbronn in these two lines line we are:

1. grouping the data in lon, lat poitns, so each group is a CTD profile;
2. getting the first group, aka, the first CTD profile.

A user may want to:
a. plot profile x by itself, not necessarily the first
b. plot all profiles for a quick inspection

For a we should introduce a new kw arg were the [0] above would the profile number.
Option b is tricky b/c we can easily blow up the memory if the section is too big.

Can try to implement option a?

[FloraSauerbronn]

Yes, I will try to come up with something today. I am still trying to understand why the pre-commit didn't work.

[ocefpaf]

You mean why pre-commit failed or why they did not work on your machine? Either way, don't worry about pre-commits until the last minute when the PR is ready to merge. We can use autofix to correct it then.

[FloraSauerbronn]

Okay.

In the kwargs of the CTD plot, I would like the user to be able to choose which latitude and longitude they want to see a profile for. However, the problem is that they might not get the specific latitude or longitude they want. If the kwargs were just a simple index, I don't know how that would help because the user wouldn't know which latitude and longitude the profile corresponds to by just choosing an index in a dataframe. There needs to be a way for the user to see which latitudes and longitudes are available and then choose accordingly.

[ocefpaf]

Let's try something simpler first. A user already selected a bounding box and is aware of the positions where the profiles lies. So, accessing profiles based on integer indices is not a bad approach. You would need to make that [0] something like [idx] and add an idx argument with the accompanying documentation for it.

[FloraSauerbronn]

Ok, feeling a bit weird about such a simple solution. I keep thinking about adding a title to the plot with the latitude and longitude, but the user can do this on their own.

[FloraSauerbronn]

Doing the push again with changes..

[ocefpaf]

Rename this to something more descriptive. Maybe profile_number?

[ocefpaf]

Describe that idx is a profile number.

Suggested change

	:param idx: index of position
	:param idx: index of position

[ocefpaf]

Pressure should be a positive number. We can invert the axis here instead with something like:

    if not ax.yaxis_inverted():
        ax.invert_yaxis()

BTW, I'm confused with ax, ax1, ax2. Aren't we plotting just a single variable on ax?

Suggested change

	ax1.plot(profile[var], -profile["pressure"], label=var, color=color)
	ax1.plot(profile[var], profile["pressure"], label=var, color=color)

[FloraSauerbronn]

We are using the command twiny for duplicating axes. I couldn't solve it just using "ax" for both, so I decided to specify.

[ocefpaf]

@FloraSauerbronn let's get this one in first before iterating on #122. Things may change there after we merge this one.

[ocefpaf]

Please correct me if I'm wrong but it is not clear to me why you renamed the ax object here.

Suggested change

	fig, ax1 = plt.subplots(figsize=(5, 6))
	ax1.plot(profile[var], profile["pressure"], label=var, color=color)
	ax1.set_ylabel("Pressure")
	ax1.set_xlabel(var)
	ax1.legend()
	ax1.invert_yaxis()
	return fig, ax1
	fig, ax = plt.subplots(figsize=(5, 6))
	ax1.plot(profile[var], profile["pressure"], label=var, color=color)
	ax.set_ylabel("Pressure")
	ax.set_xlabel(var)
	ax.legend()
	ax.invert_yaxis()
	return fig, ax

[ocefpaf]

Do we need this? We are supporting a single variable, why plot it twice?

Suggested change

	fig = ax.get_figure()
	ax2 = ax.twiny() # Create a new twinned axis
	ax2.plot(profile[var], profile["pressure"], label=var, color=color)
	ax2.set_xlabel(var)

[ocefpaf]

If there is a reason for the code above, we need. However, my feeling is that we don't need the twiny axis and this code block can also be removed.

[ocefpaf]

Note that you also need to regenerate the figures when you change them. Otherwise you will be comparing a new modified image to the old one.

Suggested change

	return fig, ax2
	return fig, ax

[FloraSauerbronn]

You're correct . I'm changing it and doing the push again.

[FloraSauerbronn]

PR was reviewed. plotting.py and test_plotting.py were altered, and a new baseline image was created for the ax in plot_ctd.

Let me know if you need any more adjustments!

[ocefpaf]

We don't need this now that we have a single ax.

Suggested change

	lines, labels = ax.get_legend_handles_labels()
	lines2, labels2 = ax.get_legend_handles_labels()
	ax.legend(lines + lines2, labels + labels2, loc="lower center")

Suggested change

	lines, labels = ax.get_legend_handles_labels()
	lines2, labels2 = ax.get_legend_handles_labels()
	ax.legend(lines + lines2, labels + labels2, loc="lower center")

[FloraSauerbronn]

The pre-commit stopped running on my computer. I don't know why, so I couldn't review the errors. I will try again later.

[ocefpaf]

The pre-commit stopped running on my computer. I don't know why, so I couldn't review the errors. I will try again later.

Ignore the pre-commit failures for now. If everything is OK after the review we can use the bot command to fix it and merge.

[ocefpaf]

This should be an if-check block outside of the ax creation. We had it before if I'm not mistaken.

[FloraSauerbronn]

The problem occurs when calling the legend while using the twiny axis. We have three scenarios:

1. When using just one axis (not referring to a second one like ax2), the legend will only display the last variable plotted if twiny is used, so we need to create twoo box one for each legend and specify where they should be placed.

`
@register_dataframe_method
def plot_ctd(
df: pd.DataFrame,
profile_number: int,
var: str,
ax: plt.Axes = None,
color: str | None = None,
) -> tuple:
"""Make a CTD profile plot of pressure vs property
depending on what variable was chosen.

:param profile_number: profile number of CTD
:param var: variable to plot against pressure
:param ax: existing axis to plot on (default: None)
:param color: color for the plot line (default: None)
:return: figure, axes
"""
g = df.groupby(["longitude", "latitude"])
profile = g.get_group(list(g.groups)[profile_number])

if ax is None:
    fig, ax = plt.subplots(figsize=(5, 6))
    ax.plot(profile[var], profile["pressure"], label=var, color=color)
    ax.set_ylabel("Pressure")
    ax.set_xlabel(var)
    ax.invert_yaxis()
    ax.legend(loc="lower center", bbox_to_anchor=(0.5, 0.07))
    return fig, ax
else:
    ax = ax.twiny()  # Create a new x-axis on top
    ax.plot(profile[var], profile["pressure"], label=var, color=color)
    ax.set_xlabel(var)
    ax.legend(loc="lower center")
    return ax.figure, ax

fig, ax = plot_ctd(df, 0, var="temperature", color="blue")
fig, ax = plot_ctd(df, 0, var="salinity", ax=ax, color="red")`

2. Or calling the legend inside each clouse (if ax is none and else) without defining each ax they would overlap eachother.

3. Or we defined each ax and call the legens like this so they can be in the same box

`@register_dataframe_method
def plot_ctd(
df: pd.DataFrame,
profile_number: int,
var: str,
ax: plt.Axes = None,
color: str | None = None,
) -> tuple:
"""Make a CTD profile plot of pressure vs property
depending on what variable was chosen.

:param profile_number: profile number of CTD
:param var: variable to plot against pressure
:param ax: existing axis to plot on (default: None)
:param color: color for the plot line (default: None)
:return: figure, axes
"""
g = df.groupby(["longitude", "latitude"])
profile = g.get_group(list(g.groups)[profile_number])

if ax is None:
    fig, ax = plt.subplots(figsize=(5, 6))
    lns1 = ax.plot(profile[var], profile["pressure"], label=var, color=color)
    ax.set_ylabel("Pressure")
    ax.set_xlabel(var)
    ax.invert_yaxis()
    
    # Get handles and labels for the legend
    handles, labels = ax.get_legend_handles_labels()
    ax.legend(handles, labels, loc="best")
    return fig, ax
else:
    ax2 = ax.twiny()  # Create a new x-axis on top
    lns2 = ax2.plot(profile[var], profile["pressure"], label=var, color=color)
    ax2.set_xlabel(var)
    
    # Get handles and labels for both axes
    handles1, labels1 = ax.get_legend_handles_labels()
    handles2, labels2 = ax2.get_legend_handles_labels()
    
    # Combine handles and labels
    handles = handles1 + handles2
    labels = labels1 + labels2
    
    # Set the combined legend
    ax.legend(handles, labels, loc="best")
    
    return ax.figure, ax2

Example usage:

fig, ax = plot_ctd(df, 0, var="temperature", color="blue")
fig, ax = plot_ctd(df, 0, var="salinity", ax=ax, color="red")
`

[ocefpaf]

🎉