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

feat: added option for title in the charts #138

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Fersoil
Copy link
Collaborator

@Fersoil Fersoil commented Nov 21, 2024

Contributing to Meteors

Thank you for contributing to Meteors! Please follow the guidelines below to ensure a smooth and efficient contribution process.

PR Checklist

  • PR Title: "semantic tag: description"

    • Package: Use the appropriate semantic tag for your PR:
      • fix: A bug fix.
      • feat: A new feature.
      • docs: Documentation only changes.
      • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc).
      • refactor: A code change that neither fixes a bug nor adds a feature.
      • perf: A code change that improves performance.
      • test: Adding missing tests or correcting existing tests.
      • chore: Changes to the build process or auxiliary tools and libraries such as documentation generation.
      • build: Changes that affect the build system or external dependencies.
    • Example: "fix: typo in README.md"
  • PR Message: Delete this entire checklist and replace it with:

    • Description: A detailed description of the change.
    • Dependencies: Any dependencies required for this change.
  • Add tests and docs: If you're adding a new integration, please include

    1. a test for the integration, preferably unit tests that do not rely on network access,
    2. an example notebook showing its use, located in the examples directory.
  • Lint and test: Run rye run pre-commit run --all-files and rye test to ensure your changes pass all checks. See contribution guidelines for more.

  • Related Issue(s): If your PR fixes an issue, please link it in the PR description.

Additional Guidelines

  • Import optional dependencies within functions.
  • Avoid adding dependencies to pyproject.toml files unless required for unit tests.
  • Most PRs should not modify more than one package.
  • Ensure changes are backwards compatible.

Review Process

If no one reviews your PR within a few days, please @-mention one of the maintainers.

@Fersoil Fersoil linked an issue Nov 21, 2024 that may be closed by this pull request
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
unittests 94.55% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/meteors/visualize/attr_visualize.py 98.90% <100.00%> (+0.04%) ⬆️
src/meteors/visualize/hsi_visualize.py 100.00% <100.00%> (ø)

🚨 Try these New Features:

@@ -71,6 +72,8 @@ def visualize_attributes(
ax[0, 0].grid(False)
ax[0, 0].axis("off")

if title is None:
title = f"HSI Attributes of: {rotated_attributes_dataclass.attributes}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

title is not assigned to anything. Additionally you need to consider if the user passed ax or None because then we can set title on fig or only on ax

spatial_attributes: HSISpatialAttributes,
ax: Axes | None = None,
use_pyplot: bool = False,
title="Spatial Attributes Visualization",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes you assign a default string and sometimes None and set the string in the code. I like when we assign a default string (less code) like here. Make it consistent please

Copy link
Collaborator Author

@Fersoil Fersoil Nov 21, 2024

Choose a reason for hiding this comment

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

What about the case of the method which is a wrapper around the others? visualize_aggregated_attributes calls two methods, and if I don't do it with None, then I would need to set the title in the wrapper method explicitly

src/meteors/visualize/attr_visualize.py Outdated Show resolved Hide resolved
@@ -107,6 +107,10 @@ def test_visualize_hsi_with_hsi_object():
# Check if the axes object is returned
assert isinstance(ax, Axes)

# Check if title is set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add also testing lines for test_attr_visualization because you missed it with one title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: axis setting
2 participants