-
Notifications
You must be signed in to change notification settings - Fork 24
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
Make Soma a single class used by both mutable and immutable #366
Open
eleftherioszisis
wants to merge
13
commits into
master
Choose a base branch
from
zisis/remove-soma-friend
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
9d0ac2b
Merge mut/readonly somata.
eleftherioszisis df6815f
Fixes
eleftherioszisis 48b2449
Add soma testing
eleftherioszisis 9f5300f
Remove mut soma files
eleftherioszisis 9bff517
Fix soma bindings
eleftherioszisis 366b1c4
clang
eleftherioszisis a07313d
Remove unused include
eleftherioszisis 4faaae3
Return NaN from empty soma computation methods
eleftherioszisis 7ff0fb8
Add cpp tests for soma
eleftherioszisis a70457c
clang
eleftherioszisis eda4350
Revert to using the soma exceptions
eleftherioszisis 741b5b7
Remove unused function
eleftherioszisis 0be6665
Use double in initializer lists
eleftherioszisis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
|
||
#include <pybind11/numpy.h> | ||
#include <pybind11/pybind11.h> | ||
#include <pybind11/stl.h> | ||
|
||
#include <morphio/soma.h> | ||
#include <morphio/types.h> | ||
|
||
#include "bind_soma.h" | ||
#include "bindings_utils.h" | ||
|
||
namespace py = pybind11; | ||
|
||
void bind_soma_module(py::module& m) { | ||
py::class_<morphio::Soma, std::shared_ptr<morphio::Soma>>(m, "Soma") | ||
.def(py::init<const morphio::Soma&>()) | ||
|
||
.def(py::init<const morphio::Property::PointLevel&>()) | ||
|
||
.def_property( | ||
"points", | ||
[](morphio::Soma* soma) { | ||
return py::array(static_cast<py::ssize_t>(soma->points().size()), | ||
soma->points().data()); | ||
}, | ||
[](morphio::Soma* soma, py::array_t<morphio::floatType> points) { | ||
soma->points() = array_to_points(points); | ||
}, | ||
"Returns the coordinates (x,y,z) of all soma point") | ||
|
||
.def_property( | ||
"diameters", | ||
[](morphio::Soma* soma) { | ||
return py::array(static_cast<py::ssize_t>(soma->diameters().size()), | ||
soma->diameters().data()); | ||
}, | ||
[](morphio::Soma* soma, py::array_t<morphio::floatType> _diameters) { | ||
soma->diameters() = _diameters.cast<std::vector<morphio::floatType>>(); | ||
}, | ||
"Returns the diameters of all soma points") | ||
|
||
.def_property_readonly( | ||
"center", | ||
[](morphio::Soma* soma) { return py::array(3, soma->center().data()); }, | ||
"Returns the center of gravity of the soma points") | ||
|
||
.def_property_readonly("max_distance", | ||
&morphio::Soma::maxDistance, | ||
"Return the maximum distance between the center of gravity " | ||
"and any of the soma points") | ||
|
||
.def_property_readonly("type", &morphio::Soma::type, "Returns the soma type") | ||
|
||
.def_property_readonly("surface", | ||
&morphio::Soma::surface, | ||
"Returns the soma surface\n\n" | ||
"Note: the soma surface computation depends on the soma type"); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
#pragma once | ||
#include <pybind11/pybind11.h> | ||
|
||
void bind_soma_module(pybind11::module& m); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
#pragma once | ||
|
||
#include <morphio/properties.h> | ||
#include <morphio/morphology.h> | ||
#include <morphio/types.h> | ||
|
||
|
@@ -25,22 +26,33 @@ namespace morphio { | |
* | ||
* @version unstable | ||
*/ | ||
class Soma | ||
struct Soma | ||
{ | ||
public: | ||
Soma() = default; | ||
Soma(const Soma& soma) = default; | ||
|
||
explicit Soma(const Property::Properties& properties); | ||
explicit Soma(const Property::PointLevel& point_properties); | ||
|
||
/// Return the coordinates (x,y,z) of all soma points | ||
range<const Point> points() const noexcept { | ||
return properties_->_somaLevel._points; | ||
std::vector<Point>& points() noexcept { | ||
return properties_._points; | ||
} | ||
const std::vector<Point>& points() const noexcept { | ||
return properties_._points; | ||
} | ||
|
||
/// Return the diameters of all soma points | ||
range<const floatType> diameters() const noexcept { | ||
return properties_->_somaLevel._diameters; | ||
std::vector<morphio::floatType>& diameters() noexcept { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
return properties_._diameters; | ||
} | ||
const std::vector<morphio::floatType>& diameters() const noexcept { | ||
return properties_._diameters; | ||
} | ||
|
||
/// Return the soma type | ||
SomaType type() const noexcept { | ||
return properties_->_cellLevel._somaType; | ||
return soma_type_; | ||
} | ||
|
||
/// Return the center of gravity of the soma points | ||
|
@@ -64,16 +76,18 @@ class Soma | |
*/ | ||
floatType maxDistance() const; | ||
|
||
/// Return soma properties | ||
Property::PointLevel& properties() noexcept { | ||
return properties_; | ||
} | ||
const Property::PointLevel& properties() const noexcept { | ||
return properties_; | ||
} | ||
|
||
private: | ||
explicit Soma(const std::shared_ptr<Property::Properties>& properties); | ||
// TODO: find out why the following line does not work | ||
// when friend class Morphology; is removed | ||
// template <typename Property> | ||
// friend const morphio::Soma morphio::Morphology::soma() const; | ||
friend class Morphology; | ||
friend class mut::Soma; | ||
|
||
std::shared_ptr<Property::Properties> properties_; | ||
SomaType soma_type_ = SOMA_UNDEFINED; | ||
Property::PointLevel properties_; | ||
}; | ||
|
||
} // namespace morphio |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So… this makes things mutable, is this still the right place then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that there is no readonly/mutable duality with these changes, I thought I might leave it in
morphio/soma.h
. But you are right, it can be moved to mut too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose one could say that the Morphology proper is the (im)mutable entity, in which case the placement of the soma would be fine. But then makes me wonder, shouldn't the section class receive the same treatment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is an experiment to check if I can pull this through. pybind11 is giving me hell at the time being...
If it will work, yes, I don't see why not do the same for sections and reduce the complexity of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although with sections, the switching from leveraging the ranges to copying the data so that a section data structure is created the same way for both mut/readonly, will result in quite a performance penalty. With soma it doesn't really matter given that it is just one and with very few points.