Skip to content

Conversation

@brandynlucca
Copy link
Collaborator

This PR includes the refactored plotting functions for the transect results, kriged mesh results, and age-length population heatmap. This also includes accompanying tests and fixtures.

@brandynlucca brandynlucca marked this pull request as ready for review September 11, 2025 18:50
@brandynlucca brandynlucca self-assigned this Sep 11, 2025
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

@brandynlucca : Thanks for the PR - the additions look good!

I couldn't find a .py file that demonstrate the use of the added plotting functions, but not sure if I miss anything since there are many files irrelevant to this PR on the nav bar.

Could you make a separate .py that loads results from feat_hake.py to generate these plots and save them as PNG in a demo folder? Be careful with the savefig arguments since sometimes without the right combination the saved fig would not have the right background or be cut off on the side, etc. Please also include demonstration of what should be included in each of the *_kwargs so that people know how to use them (aside from having those info in the docstrings). Would be great to have a couple lines about the include_filter and exclude_filter since it's a recurrent pattern in Echopop and having it everywhere would help people grasp their use.

@brandynlucca brandynlucca force-pushed the refactor_plotting_utilities branch from 39a5073 to 0ffab75 Compare September 17, 2025 17:21
@brandynlucca
Copy link
Collaborator Author

There is now a /demo/ folder in workflows that includes example outputs based on the new plotting_demo.py workflow file. After mulling this PR over a bit, I think it would be more useful in the future to further reduce the plotting functions to just return the Axes (or GeoAxes) objects and let folks add whatever they want around it. But otherwise, everything has been incorporated so I will merge this into the main branch.

@brandynlucca brandynlucca merged commit f562d16 into OSOceanAcoustics:main Sep 17, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants