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

Add CLI for spatialdata_io. #239

Merged
merged 26 commits into from
Jan 13, 2025
Merged

Conversation

migueLib
Copy link
Contributor

There was an initial commit done by @FloWuenne in the PR #72.

The commit will add :

  • CLI for each of the readers to convert to SpatialData object.
  • CLI for to convert generic formats to .zarr.

This commit should close issue #238 and #231 (? @LLehner, @LucaMarconato, @quentinblampey )
This commit is co-authored by: @chiarasch

migueLib and others added 13 commits November 13, 2024 10:20
Co-authored-by: Chiara Schiller <chiara_schiller@t-online.de>

Co-authored-by: Florian Wuennemann <flowuenne@gmail.com>
Co-authored-by: Chiara Schiller <chiara_schiller@t-online.de>
Co-authored-by: Chiara Schiller <chiara_schiller@t-online.de>
Co-authored-by: Chiara Schiller <chiara_schiller@t-online.de>
Co-authored-by: Chiara Schiller <chiara_schiller@t-online.de>
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 85.60000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 50.41%. Comparing base (c4f6cf7) to head (fe30019).
Report is 50 commits behind head on main.

Files with missing lines Patch % Lines
src/spatialdata_io/__main__.py 85.87% 25 Missing ⚠️
src/spatialdata_io/converters/generic_to_zarr.py 75.00% 7 Missing ⚠️
src/spatialdata_io/readers/generic.py 88.57% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   43.68%   50.41%   +6.72%     
==========================================
  Files          23       26       +3     
  Lines        2353     2644     +291     
==========================================
+ Hits         1028     1333     +305     
+ Misses       1325     1311      -14     
Files with missing lines Coverage Δ
src/spatialdata_io/__init__.py 100.00% <100.00%> (ø)
src/spatialdata_io/experimental/__init__.py 100.00% <100.00%> (ø)
src/spatialdata_io/readers/generic.py 88.57% <88.57%> (ø)
src/spatialdata_io/converters/generic_to_zarr.py 75.00% <75.00%> (ø)
src/spatialdata_io/__main__.py 85.87% <85.87%> (ø)

... and 2 files with indirect coverage changes

@migueLib
Copy link
Contributor Author

Forgot to mention @melonora

@LLehner LLehner changed the title This PR will add a CLI for spatialdata_io. Add CLI for spatialdata_io. Nov 14, 2024
Copy link
Contributor

@quentinblampey quentinblampey left a comment

Choose a reason for hiding this comment

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

Hi, thanks again for the PR, nice work!
I made a review, but it will still need to be also reviewed by Luca

src/spatialdata_io/__init__.py Outdated Show resolved Hide resolved
src/spatialdata_io/converters/generic_to_zarr.py Outdated Show resolved Hide resolved
src/spatialdata_io/converters/generic_to_zarr.py Outdated Show resolved Hide resolved
src/spatialdata_io/converters/generic_to_zarr.py Outdated Show resolved Hide resolved
src/spatialdata_io/__main__.py Outdated Show resolved Hide resolved
src/spatialdata_io/__main__.py Outdated Show resolved Hide resolved
src/spatialdata_io/converters/generic_to_zarr.py Outdated Show resolved Hide resolved
src/spatialdata_io/converters/generic_to_zarr.py Outdated Show resolved Hide resolved
src/spatialdata_io/__main__.py Outdated Show resolved Hide resolved
@LucaMarconato
Copy link
Member

LucaMarconato commented Jan 13, 2025

Thanks again @migueLib @chiarasch @LLehner @FloWuenne @quentinblampey.

I applied the changes suggested from @quentinblampey's code review and reviewed the code myself. I have:

  • done some refactoring in the function to read generic files (and a bit on the CLI)
  • updated the CLIs to reflect the new parameters used in visium_hd, seqfish and by adding the new macsima reader
  • I added tests for the CLI, for the generic data types, and for those readers that already had a tests (Xenium, Seqfish, Macsima)
  • I added a CLI section in the docs using (sphinx-click)

@LucaMarconato
Copy link
Member

LucaMarconato commented Jan 13, 2025

One comment:

  • for the future I would consider exploring a semi-automatic way to generate the CLI and help message from existing functions and their docstrings. I could not find any click API or library that does it for click. But together with AI I made this draft. One big caveat. I don't know if this will play around nicely with sphinx-click (automatic generation of the documentation for the CLI).
import click
import inspect


def auto_click(func):
    # Get the function's signature
    sig = inspect.signature(func)

    # Create a Click command decorator
    @click.command(help=func.__doc__)
    # Add Click options based on the function's parameters
    def wrapper(*args, **kwargs):
        return func(*args, **kwargs)

    for name, param in sig.parameters.items():
        # Determine the Click option type
        param_type = (
            click.INT if param.annotation == int else
            click.FLOAT if param.annotation == float else
            click.BOOL if param.annotation == bool else
            click.STRING
        )

        # Add the option to the wrapper
        wrapper = click.option(
            f"--{name.replace('_', '-')}",
            default=param.default if param.default is not inspect.Parameter.empty else None,
            type=param_type,
            show_default=True,
            help=f"{param.name} (type: {param.annotation.__name__})"
        )(wrapper)

    return wrapper


# Example function
@auto_click
def example_function(param1: int = 10, param2: str = "default"):
    """Example function docstring."""
    print(f"param1: {param1}, param2: {param2}")


if __name__ == "__main__":
    example_function()

Starting from this we could consider simplify the code and in this way reduce the maintenance burden (otherwise every time we modify/add/remove readers, or change their docstrings, we need to manually update the CLI).

Anyway, it's very good to have a working version for all techs that we can merge already. Thanks again!

@LucaMarconato
Copy link
Member

@HelenaLC we are going to merge this soon (I promised you I would have tagged you when the CLI would have been ready and documented). CC @vjcitn

@LucaMarconato LucaMarconato merged commit 407516c into scverse:main Jan 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants