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

V4 #1111

Merged
merged 43 commits into from
May 14, 2024
Merged

V4 #1111

merged 43 commits into from
May 14, 2024

Conversation

mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Apr 4, 2024

No description provided.

mgeplf and others added 26 commits April 25, 2022 15:32
* A heterogeneous morphology consists of zero or more homogeneous and at least one heterogeneous neurite trees extending from the soma; 
* 'heterogeneous neurite trees ' is called a 'mixed subtree' for brevity
* this is a breaking change with how NeuroM<=3.x works
* this will fix #975
Remove deprecated modules and deprecation warning. viewer module was not included to be treated separately
Switch to using the morphio readonly morphology instead of mut

Co-authored-by: Mike Gevaert <[email protected]>
Removes viewer deprecated module and hides plot_tree, plot_tree3d, plot_soma, plot_soma3d from neurom.view

Co-authored-by: Zisis Eleftherios <[email protected]>
…oint (#1030)

Use soma center as a reference point for morphology-level radial distance features.

Radial distance features on the morphology level were using
as reference point the root of each neurite. While this is ok
when per-neurite features are calculated, it is incorrect when
the radial distance features are calculated for the entire cell.

This change renders soma as the reference point for calculating
the radial distances, not the root of each neurite. When the same
features are used per-neurite the old behavior of using the neurite
root is still the same.

Co-authored-by: Adrien Berchet <[email protected]>
* Updating copyright year (#1028)

Co-authored-by: Alexander Dietz <[email protected]>

* Updating copyright year (#1029)

Co-authored-by: Alexander Dietz <[email protected]>

* Remove duplicated deps jinja, sphinx (#1043)

Co-authored-by: bbpgithubaudit <[email protected]>
Co-authored-by: Alexander Dietz <[email protected]>
Co-authored-by: stefanoantonel <[email protected]>
Make file paths absolute in Population init, but not resolve symlinks.

Co-authored-by: Adrien Berchet <[email protected]>
* Maintain mut/immut after transformation
* Add tests

Co-authored-by: Eleftherios Zisis <[email protected]>
* Updating copyright year (#1028)

Co-authored-by: Alexander Dietz <[email protected]>

* Updating copyright year (#1029)

Co-authored-by: Alexander Dietz <[email protected]>

* Remove duplicated deps jinja, sphinx (#1043)

* remove single point contour somas in h5 and asc tests (#1045)

* this is in prep for MorphIO to be more consistent and picky with contour somas

* Fix view cli (#1051)

* Fix coverage (#1056)

Co-authored-by: bbpgithubaudit <[email protected]>
Co-authored-by: Alexander Dietz <[email protected]>
Co-authored-by: stefanoantonel <[email protected]>
Co-authored-by: MikeG <[email protected]>
Co-authored-by: Alexis Arnaudon <[email protected]>
---------

Co-authored-by: Eleftherios Zisis <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a44ecc4) to head (69a0073).

❗ Current head 69a0073 differs from pull request most recent head ab906d1. Consider uploading reports for the commit ab906d1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            master     #1111    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           36        35     -1     
  Lines         2473      2610   +137     
==========================================
+ Hits          2473      2610   +137     

@eleftherioszisis
Copy link
Contributor

eleftherioszisis commented Apr 5, 2024

The register/unregister in NeuriteType enum in this branch is quite fragile because it tries to bend the immutability of the python enums:

    @classmethod
    def register(cls, name, value):
        """Register a new value in the Enum class."""
        value = _int_or_tuple(value)

        if hasattr(cls, name):
            existing = getattr(cls, name)
            raise ValueError(f"NeuriteType '{name}' is already registered as {repr(existing)}")

        try:
            existing = cls(value)
        except ValueError:
            existing = None

        if existing:
            raise ValueError(f"NeuriteType '{name}' is already registered as {repr(existing)}")

        obj = _create_neurite_type(cls, value, name=name)

        cls._value2member_map_[value] = obj
        cls._member_map_[name] = obj
        cls._member_names_.append(name)

        return obj

    @classmethod
    def unregister(cls, name):
        """Unregister a value in the Enum class."""
        if name not in cls._member_names_:
            raise ValueError(
                f"The NeuriteType '{name}' is not registered so it can not be unregistered"
            )

        value = cls._member_map_[name].value
        del cls._value2member_map_[value]
        del cls._member_map_[name]
        cls._member_names_.remove(name)

Therefore, it is no surprise that some of the functionality we relied upon is now broken in 3.12 because they tried to make them safer: https://docs.python.org/3/whatsnew/3.12.html#enum

In addition, we rely on protected variables to make it work, which is also not great.

In the end, the use cases for which registration is needed are not so many. To make an int type usable, it has to also exist in MorphiIO as a SectionType and all section types are already available as neurite types.

That leaves us with registering new composite types that combine existing section types. As we don't have use cases where other type combinations are needed apart from axon-on-dendrite, which is already defined in NeuriteType, I suggest we remove the registration of neurite types and revisit them in the future when solid cases for this functionality appear.

eleftherioszisis and others added 12 commits April 5, 2024 15:00
* Fix NeuriteType for Python 3.12.3

* Coverage
…_segments (#1054)

* Improve iter_segments and add segment methods in core objects

* Rename iterator_type into section_iterator

* Remove iter_* methods and use consistent properties

* Use iter_sections instead of obj.sections in check.morphtree

* Update doc
* Expose soma methods as free functions.
* Allow using both morphio and neurom soma in soma functions.
* Keep backward compatibility with the soma methods.
* Disallow storing soma values to avoid issues with mutation of soma.
---------

Co-authored-by: Adrien Berchet <[email protected]>
Copy link
Collaborator Author

@mgeplf mgeplf left a comment

Choose a reason for hiding this comment

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

I've read a lot of it, and it all looks good to me.

If converting our other tools hasn't produced any large WAT moments, I think we should go with this.

@eleftherioszisis eleftherioszisis changed the title WIP: V4 V4 May 14, 2024
@eleftherioszisis eleftherioszisis merged commit 548e054 into master May 14, 2024
5 checks passed
@eleftherioszisis eleftherioszisis deleted the v4 branch May 14, 2024 07:58
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.

4 participants