-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: master
Are you sure you want to change the base?
Conversation
include/morphio/soma.h
Outdated
@@ -65,15 +65,10 @@ class Soma | |||
*/ | |||
floatType maxDistance() const; | |||
|
|||
private: | |||
explicit Soma(const std::shared_ptr<Property::Properties>&); |
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 believe that it was done this way b/c the public interface is smaller; which means less potentially breaking changes and ppl depending on certain implementation details. (ie: Hyrum's Law); that's why I'd lean to not make it public.
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.
However, resorting to using friends to bypass the private constructor seems like an unnecessary complexity to me, isn't it?
d820c4c
to
a5b7df2
Compare
8438807
to
e10cf83
Compare
e10cf83
to
9f5300f
Compare
/// 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 { |
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.
} | ||
|
||
/// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -18,11 +21,20 @@ Soma::Soma(const Property::PointLevel& point_properties) | |||
|
|||
|
|||
Point Soma::center() const { | |||
|
|||
if (points().empty()) { | |||
return {NaN, NaN, NaN}; |
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.
Do people check for NaN? If I use Python, I would just expect this to return None
, and by now, in compiled languages, expect std::optional
(or language equivalent)
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.
yeah, I removed them after thinking twice and also realizing that the volume/surface functions already raise exceptions.
Summary: