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

Creating new function plot_ts for TS diagram #125

Merged
merged 10 commits into from
Oct 29, 2024

Conversation

FloraSauerbronn
Copy link
Contributor

New function plot_ts
Updated plots
Updated tests
Updated imagebaseline

@FloraSauerbronn
Copy link
Contributor Author

This pre-commit is really hard...
I created #126 to fix the old name of plot_ctd.

After it is merged, I will continue trying to add plot_ts.
@ocefpaf, is there another name you think would better suit this function?

@ocefpaf
Copy link
Member

ocefpaf commented Sep 19, 2024

This pre-commit is really hard... I created #126 to fix the old name of plot_ctd.

Ignore it until the PR is done. Then we can fix the pre-commits. It helps old developers like myself to handle projects when many contributors but it sure increases barrier for contributions.

After it is merged, I will continue trying to add plot_ts.

I merged #126 and rebased this one. You will need to perform a git pull on this PR branch locally to get the latest merged commit. Let me know if you need help with that.

@ocefpaf, is there another name you think would better suit this function?

I believe that plot_ts is fine. Let me know when this one is ready for review!

df: pd.DataFrame,
profile_number: int,
) -> tuple[plt.Figure, plt.Axes]:
"""Make a TS diagram from a chosen profile number.
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 do the TS for the whole track. TS diagrams have many uses and the very first one is tp assess data quality, find oddities, etc. I believe that this function will be used to for quick data inspection like that, not really a final figure for a paper or refined research.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ocefpaf , to create a plot with all the profiles, I would need to interpolate them so that they are in the same pressure intervals, adding NaN where the profiles are shallower than others. Do my thoughts make sense, or is there a simpler way?

@FloraSauerbronn
Copy link
Contributor Author

@ocefpaf 🙌

@ocefpaf
Copy link
Member

ocefpaf commented Oct 29, 2024

I rebased it and remove the extra baseline directory inside the module. We keep the tests outside of it b/c we don't want to ship these PNGs with the package.

@ocefpaf ocefpaf merged commit 6ee9dea into ioos:main Oct 29, 2024
15 checks passed
@ocefpaf ocefpaf mentioned this pull request Oct 29, 2024
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.

2 participants