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

adjust to xarray datatree #94

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Conversation

melonora
Copy link
Collaborator

@melonora melonora commented Nov 1, 2024

closes #93

Xarray has archived the datatree repository, the new import from xarray import datatree and from_dict only accepts positional arguments from now on. Another change is that from_dict does not accept DataArray as values anymore, but only xr.Dataset. With this PR and adjustment in SpatialData scverse/spatialdata#752, it seems to work.

This PR is not meant to be merged yet as I am still checking on the changes in SpatialData. I currently just only have a windows machine which does not allow for some tests.

@melonora
Copy link
Collaborator Author

melonora commented Nov 3, 2024

@thewtex any idea regarding pixi here?

@melonora
Copy link
Collaborator Author

melonora commented Nov 3, 2024

I did install with pixi but updating the lockfile there is the issue of xarray requiring python version being between 3.10 and 3.13.. After that there are a few more dependencies that need to be solved. Question are we ok with dropping 3.9 support at this point @LucaMarconato @thewtex ?

@melonora
Copy link
Collaborator Author

melonora commented Nov 4, 2024

If everyone is ok with dropping 3.9 support then I think we would be good to go. I am only implementing some small changes in napari-spatialdata still, but in the main spatialdata repo things seem to be ok.

@melonora
Copy link
Collaborator Author

melonora commented Nov 4, 2024

One thing to note is that methods like transpose are not available at the DataTree level anymore so map_over_datasets has to be used. For this a decorator was required as we have a root node not containing anything causing many of the functions to fail as map_over_datasets does not allow for skipping a node.

@thewtex
Copy link
Contributor

thewtex commented Nov 4, 2024

@melonora awesome, thanks for the updates!!

Dropping 3.9 would be fine with me.

Why does 3.13 not work?

Could the CI config please be updated to reflect the supported Python versions?

@melonora
Copy link
Collaborator Author

melonora commented Nov 4, 2024

@melonora awesome, thanks for the updates!!

Dropping 3.9 would be fine with me.

Why does 3.13 not work?

Could the CI config please be updated to reflect the supported Python versions?

Just checked again, yesterday I perhaps did something different with the constraints so it started including release candidates which caused it to fail. However, retrying today everything seems A-ok so must have been doing something different but I don't remember.

Sure updating CI config now.

Copy link
Contributor

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

💯 🎇 thank you @melonora !

@melonora
Copy link
Collaborator Author

melonora commented Nov 5, 2024

I did not see a changelog so I am not certain for release whether that is manually written or automatically handled. Anything still required before merging and making the release?

@thewtex
Copy link
Contributor

thewtex commented Nov 5, 2024

Release notes are generated from Git logs. I think we should make a major bump in the version add a special note regarding the datatree bump.

@thewtex thewtex merged commit 05c5696 into spatial-image:main Nov 5, 2024
17 checks passed
@TomNicholas
Copy link

Glad you got this working with the new xarray.DataTree!

the new import from xarray import datatree and from_dict only accepts positional arguments from now on. Another change is that from_dict does not accept DataArray as values anymore, but only xr.Dataset

Did we miss these from the migration guide? Specifically that DataTree.from_dict only accepts positional args, and doesn't accept DataArray?

@melonora
Copy link
Collaborator Author

melonora commented Nov 7, 2024

Glad you got this working with the new xarray.DataTree!

the new import from xarray import datatree and from_dict only accepts positional arguments from now on. Another change is that from_dict does not accept DataArray as values anymore, but only xr.Dataset

Did we miss these from the migration guide? Specifically that DataTree.from_dict only accepts positional args, and doesn't accept DataArray?

@TomNicholas , sorry for not getting back to you sooner. Previously the keyword argument to DataTree.from_dict d=<dictionary> could be passed that way and it was implemented as such in this library. Small change but I did not see this one in the API changes of the migration guide, I can open a PR for that though.

Same for DataArray, we have an upper constraint in SpatialData on the xarray version for now but will implement the PRs that I opened, but some example current code is now like this:

        # Line below does not work anymore with the latest xarray
        transformed_dict[k] = DataArray(transformed_dask, dims=xdata.dims, name=xdata.name, attrs={TRANSFORM_KEY: {}})
        
        #Line below is what will be implemented
        transformed_dict[k] = Dataset(
            {"image": DataArray(transformed_dask, dims=xdata.dims, name=xdata.name, attrs={TRANSFORM_KEY: {}})}
        )
        if channel_names is not None:
            # This expression returns a dataset now.
            transformed_dict[k] = transformed_dict[k].assign_coords(c=channel_names)

    # mypy thinks that schema could be ShapesModel, PointsModel, ...
    transformed_data = DataTree.from_dict(transformed_dict)

This was a bit confusing as appearently you still have the method DataTree.to_dataarray. I will open a few issues in the xarray repo to see what the overall feedback is on it as I have some other questions too and if required I can open a few PRs

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.

Datatree functionality is in Xarray
3 participants