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

feat: Stack regions and Layer partitions #868

Merged
merged 16 commits into from
Sep 13, 2024
Merged

feat: Stack regions and Layer partitions #868

merged 16 commits into from
Sep 13, 2024

Conversation

drodarie
Copy link
Contributor

@drodarie drodarie commented Jul 25, 2024

Describe the work done

Stack will now use its children's stack_index attribute to order them in the stack before repositioning them.
In practice, a Layer of a Stack can "decide" of its position within it.

List which issues this resolves:

Closes #867

Tasks

  • Added tests

📚 Documentation preview 📚: https://bsb--868.org.readthedocs.build/en/868/

@drodarie drodarie requested a review from Helveg July 25, 2024 12:58
@drodarie drodarie self-assigned this Jul 25, 2024
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

I gave notes to change the UX of the Stack with more feature parity to what stacks were in BSB3, and what I think is a more rich and intuitive user experience.

bsb/topology/partition.py Show resolved Hide resolved
bsb/topology/region.py Outdated Show resolved Hide resolved
bsb/topology/region.py Outdated Show resolved Hide resolved
bsb/topology/region.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the fix label Aug 1, 2024
@drodarie drodarie changed the title fix: Stack regions and Layer partitions feat: Stack regions and Layer partitions Aug 1, 2024
@github-actions github-actions bot added feat and removed fix labels Aug 1, 2024
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

just noticed that anchor refs any partition/region

"""

axis: typing.Union[typing.Literal["x"], typing.Literal["y"], typing.Literal["z"]] = (
config.attr(type=types.in_(["x", "y", "z"]), default="z")
)
stack_order: list[typing.Union["Region", "Partition"]] = config.reflist(
refs.regional_ref, backref="region"
)
anchor: typing.Union["Region", "Partition"] = config.ref(
refs.regional_ref, backref="region"
Copy link
Contributor

Choose a reason for hiding this comment

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

Atm the ref can be any partition/region, even non-children. This can either be:

  • allowed and your intention: this would allow "out of stack anchors", but then we need to drop the backref="region" (backref would set the target's region to this region, effectively transferring/stealing it from its parent)
  • not your intention: swap to a new ref function to strictly one of the children (not sure if a ref to a ref works, probably untested)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never thought of making a ref to a ref 😮 but I managed to make it work with just a ref. The function _resolve_anchor_offset should check if the ref belongs to the children. I can also add a test for that.

@drodarie
Copy link
Contributor Author

drodarie commented Aug 5, 2024

@Helveg what is the purpose of volume_scale in the Layer class. It does not seem to be used at the moment?

@Helveg
Copy link
Contributor

Helveg commented Aug 5, 2024

It was a feature to scale the dimensions of one layer according to the volume of another layer, so that you could have e.g. 20^mm3 of IO per 200^mm3 of cerebellar cortex, see #191

We can remove it until it is reimplemented :)

@drodarie
Copy link
Contributor Author

drodarie commented Aug 6, 2024

I am not sure how you wanted to implement it. At the moment, volume_scale is a list of 2 floats?
If I understand your feature correctly, I would need to provide the ref Partition and the volume ratio. so I guess one of the float serves as a way to fetch the ref partition?

@Helveg
Copy link
Contributor

Helveg commented Aug 13, 2024

I would not advise you to implement volume_scale at this point, I think we can just remove it. The floats were originally ratios of the dimensions. Let's say you volume scale to another ref with the floats to [1, 1, 1] then you'd create a cube scaled to the ref, when you'd [1, 2, 1], the second dimension would be twice as large as the other 2 dimensions. So they'd essentially determine the shape of the scaled volume.

If there were only 2 floats, perhaps the third value was always ommited andassumed to be the axial dimension, and was always 1 as a reference point? I don't know, but that's what I remember of the feature.

I think this feature was used by @AliceGem to scale the IO layer. I'm not sure who works on cereb-IO models now, and whether they require this feature? If anyone working on that model wants to move to BSB4, and requires this feature, we can reimplement it from the git history.

Small pass on core.py documentation
@drodarie
Copy link
Contributor Author

Needs dbbs-lab/bsb-arbor#3 to be merged.

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.85%. Comparing base (f1d26b2) to head (938274c).
Report is 21 commits behind head on main.

Files Patch % Lines
bsb/topology/partition.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
+ Coverage   87.89%   88.85%   +0.95%     
==========================================
  Files         103      110       +7     
  Lines       13771    15088    +1317     
==========================================
+ Hits        12104    13406    +1302     
- Misses       1667     1682      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drodarie drodarie requested a review from Helveg August 28, 2024 14:26
bsb/core.py Outdated Show resolved Hide resolved
@drodarie drodarie merged commit 9f4046a into main Sep 13, 2024
10 checks passed
@drodarie drodarie deleted the fix/layer_stack branch September 13, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stack index is not taken into account
2 participants