dbbs-lab / bsb-core

The Brain Scaffold Builder
https://bsb.readthedocs.io
GNU General Public License v3.0
22 stars 16 forks source link

Feature/orientations #674

Closed drodarie closed 1 year ago

drodarie commented 1 year ago

Added rotation distributor using a voxel based orientation field.

Helveg commented 1 year ago

Thanks for the PR! Ignore the failing tests, that's related to a mismatch between the publicly available bsb-hdf5/main and the branch bsb-hdf5/file-dependencies that this branch interops with.

Does the orientation distributor currently work?

drodarie commented 1 year ago

Yes, I have tested it with a toy annotation and orientation example (see test_distributors.py). It seems to assign the correct orientation vectors to each position.

However, I wasn't sure of in which format to store the angles so I kept them in Euler for now. I will check other rotation functions to see which angle format was used.

Helveg commented 1 year ago

IIRC you can return either a RotationSet, a list of scipy.transformation.Rotation objects, or a (Nx3) matrix of Euler rotations in degrees. But this still needs to be documented.

drodarie commented 1 year ago

@Helveg, with my most recent commit (90c59f3) I tried to return a list of scipy.spatial.transformation.Rotation objects as you suggested but I faced the following issue:

TypeError: float() argument must be a string or a number, not 'scipy.spatial.transform._rotation.Rotation'
The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/path/to/bsb/tests/test_distributors.py", line 135, in test_distribute
    self.netw.compile(clear=True)
  File "/path/to/bsb/bsb/profiling.py", line 157, in decorated
    return f(*args, **kwargs)
  File "/path/to/bsb/bsb/core.py", line 371, in compile
    self.run_placement(p_strats)
  File "/path/to/bsb/bsb/profiling.py", line 157, in decorated
    return f(*args, **kwargs)
  File "/path/to/bsb/bsb/core.py", line 261, in run_placement
    pool.execute(loop)
  File "/path/to/bsb/bsb/services/pool.py", line 288, in execute
    job.execute(self.owner, job.f, job._args, job._kwargs)
  File "/path/to/bsb/bsb/services/pool.py", line 164, in execute
    return f(placement, *args[1:], indicators, **kwargs)
  File "/path/to/bsb/bsb/profiling.py", line 157, in decorated
    return f(*args, **kwargs)
  File "/path/to/bsb/bsb/profiling.py", line 157, in decorated
    return f(*args, **kwargs)
  File "/path/to/bsb/bsb/profiling.py", line 157, in decorated
    return f(*args, **kwargs)
  File "/path/to/bsb/bsb/placement/particle.py", line 62, in place
    self._extract_system(system, chunk, indicators)
  File "/path/to/bsb/bsb/placement/particle.py", line 51, in _extract_system
    self.place_cells(indicator, positions, chunk)
  File "/path/to/bsb/bsb/placement/strategy.py", line 89, in place_cells
    self.scaffold.place_cells(
  File "/path/to/bsb/bsb/core.py", line 433, in place_cells
    self.get_placement_set(cell_type).append_data(
  File "/path/to/bsb-hdf5/bsb_hdf5/resource.py", line 44, in handle_indirection
    return f(*bound.args, **bound.kwargs)
  File "/path/to/bsb-hdf5/bsb_hdf5/placement_set.py", line 257, in append_data
    self._rotation_chunks.append(chunk, rotations)
  File "/path/to/bsb-hdf5/bsb_hdf5/resource.py", line 44, in handle_indirection
    return f(*bound.args, **bound.kwargs)
  File "/path/to/bsb-hdf5/bsb_hdf5/chunks.py", line 248, in append
    dset[start_pos:] = data
  File "h5py/_objects.pyx", line 54, in h5py._objects.with_phil.wrapper
  File "h5py/_objects.pyx", line 55, in h5py._objects.with_phil.wrapper
  File "/path/to/venv/lib/python3.8/site-packages/h5py/_hl/dataset.py", line 902, in __setitem__
    val = numpy.asarray(val, order='C', dtype=dt)
ValueError: setting an array element with a sequence.
drodarie commented 1 year ago

@Helveg I am struggling with the output type that is expected by the function distribute of a Rotation distributor If I understand the documentation correctly, it should return a RotationSet or a list of Rotation but then bsb hdf5 does not handle these types correctly. Have I misunderstood the docs?

codecov[bot] commented 1 year ago

Codecov Report

Merging #674 (0ef4f15) into main (929445c) will increase coverage by 0.59%. The diff coverage is 100.00%.

:exclamation: Current head 0ef4f15 differs from pull request most recent head 7ab07b0. Consider uploading reports for the commit 7ab07b0 to get more accurate results

@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
+ Coverage   72.92%   73.52%   +0.59%     
==========================================
  Files         133      133              
  Lines       15023    15110      +87     
==========================================
+ Hits        10956    11109     +153     
+ Misses       4067     4001      -66     
Impacted Files Coverage Δ
bsb/_util.py 63.15% <100.00%> (+3.99%) :arrow_up:
bsb/config/_config.py 87.32% <100.00%> (ø)
bsb/morphologies/__init__.py 80.77% <100.00%> (+0.17%) :arrow_up:
bsb/placement/distributor.py 93.95% <100.00%> (+1.04%) :arrow_up:
tests/test_distributors.py 100.00% <100.00%> (ø)
tests/test_morphologies.py 99.85% <100.00%> (+<0.01%) :arrow_up:
tests/test_util.py 100.00% <100.00%> (ø)
bsb/core.py 68.98% <0.00%> (+0.26%) :arrow_up:
bsb/config/_make.py 93.68% <0.00%> (+1.01%) :arrow_up:
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Helveg commented 1 year ago

Difficulties with the PR. Closing and reopening