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

Proposal for v4 composite types #1071

Merged
merged 45 commits into from
Jul 6, 2023
Merged

Proposal for v4 composite types #1071

merged 45 commits into from
Jul 6, 2023

Conversation

adrien-berchet
Copy link
Member

This PR is the continuation of #1067

@adrien-berchet adrien-berchet changed the base branch from master to v4 April 3, 2023 07:40
@adrien-berchet adrien-berchet mentioned this pull request Apr 3, 2023
neurom/core/types.py Outdated Show resolved Hide resolved
neurom/core/types.py Outdated Show resolved Hide resolved
neurom/core/types.py Outdated Show resolved Hide resolved
neurom/core/types.py Outdated Show resolved Hide resolved
@adrien-berchet adrien-berchet force-pushed the abe/v4-composite-types branch from fc7fc32 to 6fee616 Compare May 1, 2023 16:48
@adrien-berchet adrien-berchet force-pushed the abe/v4-composite-types branch 2 times, most recently from 07cfb98 to 4729f15 Compare May 1, 2023 17:55
@adrien-berchet adrien-berchet force-pushed the abe/v4-composite-types branch from 4729f15 to a12dc2f Compare May 1, 2023 18:21
neurom/core/types.py Outdated Show resolved Hide resolved
@adrien-berchet adrien-berchet force-pushed the abe/v4-composite-types branch from d2a6509 to 3c98024 Compare May 2, 2023 14:34
Copy link
Member Author

@adrien-berchet adrien-berchet left a comment

Choose a reason for hiding this comment

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

Interesting!
So if I'm correct you discarded the following features:

  1. get neurite types by name using the ctor (e.g. NeuriteType("axon"))
  2. represent unknown types (since there is no dedicated subtype)
  3. pass several types to neurom.features.neurite._map_sections

Did I miss anything?

((3, 2), "<NeuriteType.axon_carrying_dendrite: (3, 2)>"),
],
)
def test_neurite_type__repr(value, expected):
Copy link
Member Author

Choose a reason for hiding this comment

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

You don't like nested tests? Sometimes it's convenient to run only the tests of the NeuriteType for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not so much, no. I prefer a flat listing of the tests, but this is a matter of preference mostly.

@eleftherioszisis eleftherioszisis self-requested a review July 6, 2023 07:36
Copy link
Contributor

@eleftherioszisis eleftherioszisis left a comment

Choose a reason for hiding this comment

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

Let's do it.

@adrien-berchet adrien-berchet added this to the v4 milestone Jul 6, 2023
@eleftherioszisis eleftherioszisis merged commit 1a5453e into v4 Jul 6, 2023
@eleftherioszisis eleftherioszisis deleted the abe/v4-composite-types branch July 6, 2023 07:41
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.

3 participants