Skip to content

Add CLI for spatialdata_io. #239

Merged
LucaMarconato merged 26 commits into
scverse:mainfrom
migueLib:cli-2spatialdata
Jan 13, 2025
Merged

Add CLI for spatialdata_io. #239
LucaMarconato merged 26 commits into
scverse:mainfrom
migueLib:cli-2spatialdata

Conversation

@migueLib

Copy link
Copy Markdown
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

codecov-commenter commented Nov 14, 2024

Copy link
Copy Markdown

Codecov Report

❌ 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 189 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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@migueLib

Copy link
Copy Markdown
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

@quentinblampey quentinblampey left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/spatialdata_io/__init__.py Outdated
Comment thread src/spatialdata_io/converters/generic_to_zarr.py Outdated
Comment thread src/spatialdata_io/converters/generic_to_zarr.py Outdated
Comment thread src/spatialdata_io/converters/generic_to_zarr.py Outdated
Comment thread src/spatialdata_io/__main__.py Outdated
Comment thread src/spatialdata_io/__main__.py Outdated
Comment thread src/spatialdata_io/converters/generic_to_zarr.py Outdated
Comment thread src/spatialdata_io/converters/generic_to_zarr.py Outdated
Comment thread src/spatialdata_io/__main__.py Outdated
@LucaMarconato

LucaMarconato commented Jan 13, 2025

Copy link
Copy Markdown
Member

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

LucaMarconato commented Jan 13, 2025

Copy link
Copy Markdown
Member

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
Copy Markdown
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
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.

CLI for conversion using standard technology readers Add CLI for conversions from and to standard types (.png, .tiff, .geojson, ...)

5 participants