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

Added elliptical mask and a function to obtain GIDs inside masked area #664

Merged
merged 21 commits into from Mar 17, 2017

Conversation

@stinebuu
Copy link
Contributor

stinebuu commented Feb 27, 2017

This work is done in preparation for a connectivity app that is being developed.

The PR contains

  • Elliptical mask in 2D
  • A function to obtain all the GIDs inside a masked area.
Copy link
Contributor

heplesser left a comment

@stinebuu This is a good start, but see my suggestions in the code. Could you also add tests (in Python) for the ellipse mask and the selection function?

* @param x_side Length between center and x vertex
* @param y_side Length between center and y vertex
*/
EllipseMask( Position< D > center, double x_side, double y_side )

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

At present, the mask is limited to ellipses with axis parallel to the coordinate axis. We should support ellipses with other orientations as well, with an extra parameter giving the angle between x-axis and the major axis of the ellipse (angle in radians).

Parameter names should be closer to standard mathematical terms (major/minor axis).

This comment has been minimized.

@stinebuu

stinebuu Mar 3, 2017 Author Contributor

@heplesser Thanks for your comments! We called the parameter names x_side and y_side because when we check if a point is inside the ellipse (in EllipseMask< D >::inside) we use the ellipse equation (x-x_center)^2 / x_side^2 + (y-y_center)^2 / y_side^2 <= 1, and it is important that we use the x-axis radius when dividing with x and same with y. I am unsure how this should be done with major/minor axis, because we then no longer know if the major axis comes from the x-axis or the y-axis. Do you have any suggestions as to how I could deal with this?

inline Name
EllipseMask< 2 >::get_name()
{
return "ellipse";

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

All strings should be defined in names, in topology_names.{h,cpp}. For BallMask, get_name() returns circular/spherical, so we should use adjective form here as well (elliptical).

inline Name
EllipseMask< 3 >::get_name()
{
return "ellipsoid";

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

See above, ellipsoidal.

// Currently EllipseMask only works in 2 dimensions.
if ( D != 2 )
{
throw NotImplemented( "" );

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

You should handle this not via if but by template specialization. How difficult would it be to also implement this for 3D? It would be nice to cover that from the start as well.

if ( x_side_ <= 0 or y_side_ <= 0 )
throw BadProperty(
"topology::EllipseMask<D>: "
"side > 0 required." );

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

"All axis > 0 required."

@@ -158,6 +158,11 @@

/get [/masktype /literaltype] {exch cvdict_M exch get} def
/get [/masktype /arraytype] {exch cvdict_M exch get} def

/SelectNodesByMask [/doubletype /doubletype /masktype /integertype]
/SelectNodesByMask_L_M_d_d load

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

The anchor position should be supplied as an array or doublevector, so that we can support 2D and 3D with one function.

@@ -369,6 +368,9 @@ TopologyModule::init( SLIInterpreter* i )

i->createcommand( "cvdict_M", &cvdict_Mfunction );

i->createcommand(
"SelectNodesByMask_L_M_d_d", &selectnodesbymask_L_M_d_dfunction );

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

Change arguments to anchor can be passed as array or vector.

@@ -381,6 +383,8 @@ TopologyModule::init( SLIInterpreter* i )
// Register mask types
register_mask< BallMask< 2 > >();
register_mask< BallMask< 3 > >();
register_mask< EllipseMask< 2 > >();
register_mask< EllipseMask< 3 > >();

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

Until EllipseMask supports 3D, the 3D variant should not be registered.

const index& layer_gid = getValue< long >( i->OStack.pick( 0 ) );
MaskDatum mask = getValue< MaskDatum >( i->OStack.pick( 1 ) );
double xpos = getValue< double >( i->OStack.pick( 2 ) );
double ypos = getValue< double >( i->OStack.pick( 3 ) );

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

Change to anchor as array instead of xpos and ypos. Also, I think the following order of arguments would be more logical: layer - anchor - mask, reading it as "from layer around anchor select all in mask".

double xpos = getValue< double >( i->OStack.pick( 2 ) );
double ypos = getValue< double >( i->OStack.pick( 3 ) );

Layer< 2 >* l =

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

Code here must support 2D and 3D layers and masks.

stinebuu added 5 commits Mar 8, 2017
Added tests for SelectNodesByMask and the elliptical mask. Changed name
of 'x_side' and 'y_side' to 'major_axis' and 'minor_axis' in elliptical
mask. Made it possible to have a tilted ellipse mask. Added possibility
of ellipsoidal mask. Made sure we can plot ellipse mask with
PlotTargets.
semi-axis, introduced create_bbox() and polar_angle_ (not yet
implemented) and changed some variable names.
Copy link
Contributor

heplesser left a comment

Some more comments.

, y_scale_( 4.0 / ( minor_axis_ * minor_axis_ ) )
, z_scale_( 4.0 / ( polar_axis_ * polar_axis_ ) )
, azimuth_cos_value_( cos( azimuth_angle_ ) )
, azimuth_sin_value_( sin( azimuth_angle_ ) )

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

You could drop value_ here.

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

All calls to mathematical functions should be std::cos(), std::sin(), ...

cosine_value_ = cos( angle_ );
sine_value_ = sin( angle_ );
azimuth_cos_value_ = cos( azimuth_angle_ );
azimuth_sin_value_ = sin( azimuth_angle_ );

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

std::

// If the ellipse or ellipsoid is tilted, we make the boundary box
// quadratic, with the length of the sides equal to the axis with greatest
// length. This could be more refined.
double greatest_semi_axis = std::max( major_axis_, polar_axis_ ) / 2.0;

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

Can be const

/*
* @returns boundary box of ellipse/ellipsoid.
*/
void create_bbox();

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

  • This does not return the bbox, it sets it; the comment needs to be adapted.
  • Since it is a private member function, its name should end with an _
  • It is confusing to mix methods and data members; it would be better to move this declaration before the data member declarations.

cosine_value_ = cos( angle_ );
sine_value_ = sin( angle_ );
create_bbox();

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

Parameter checks are missing here (positive axis, minor <= major).

}
else
{
z_dividend_ = 0.0;
z_scale_ = 0.0;

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

Should't polar_axis_ be set to some dummy value, to? I think 0 makes most sense.

// Currently only checks if the box is outside the bounding box of
// the ellipse. This could be made more refined.

Box< D > bb = bbox_;

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

Could be a const& to avoid copying.

@@ -430,28 +466,7 @@ template < int D >
Box< D >

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

This should return a const& to avoid copying.

width = mask['elliptical']['major_axis']
height = mask['elliptical']['minor_axis']
if 'azimuth_angle' in mask['elliptical']:
angle = mask['elliptical']['azimuth_angle']*180/pi

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

I just realized that in all stimulus generators with sinusoids, we specify the phase in degrees. So I think we should use angles in degrees as well in masks.

* @param polar_axis Length of polar axis of ellipsoid
* @param azimuth_angle Angle between x-axis and major axis of the ellipse or
* ellipsoid
* @param polar_angle Angle between z-axis and polar axis of the ellipsoid

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

After looking at sinusoidal stimulus generators, where we specify phase in degrees, I think we should specify angles in degree here as well, and then convert to radians below.

Copy link
Contributor

heplesser left a comment

Just a little bit left.

azimuth_cos_ = std::cos( azimuth_angle_ * pi_ / 180 );
azimuth_sin_ = std::sin( azimuth_angle_ * pi_ / 180 );
polar_cos_ = std::cos( polar_angle_ * pi_ / 180 );
polar_sin_ = std::sin( polar_angle_ * pi_ / 180 );

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

I would use 180. here just to be 100% sure we use floating point division.

@@ -306,13 +306,20 @@ template <>
bool
EllipseMask< 3 >::inside( const Position< 3 >& p ) const
{
const double new_x = ( p[ 0 ] - center_[ 0 ] ) * azimuth_cos_value_
+ ( p[ 1 ] - center_[ 1 ] ) * azimuth_sin_value_;
// xx = (x-x_c)*cos(azimuth) + (y-y_c)*sin(azimuth) tilts x-y plane

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

Maybe not a bad idea to add a reference?

@@ -339,13 +340,35 @@ class EllipseMask : public Mask< D >
, polar_axis_( polar_axis )
, azimuth_angle_( azimuth_angle )
, polar_angle_( polar_angle )
, pi_( 3.141592653589793 )

This comment has been minimized.

@heplesser

heplesser Mar 14, 2017 Contributor

It is better to use numerics::pi from numerics.h for consistency across the code.

Copy link
Contributor

jhnnsnk left a comment

Thanks a lot for this work. I added some comments directly in the code, mainly on documentation and typos.

Each source node should then connect to:
- The node in the same position in target layer
- The node to the left and right of that position
- The nodes above and belove.

This comment has been minimized.

@jhnnsnk

jhnnsnk Mar 15, 2017 Contributor

belove -> below

def test_CreateEllipticalMask2D(self):
"""Creates simple elliptical mask"""
mask_dict = {'major_axis': 6.0, 'minor_axis': 3.0}
mask = topo.CreateMask('elliptical', mask_dict)

This comment has been minimized.

@jhnnsnk

jhnnsnk Mar 15, 2017 Contributor

Please add the new masktype 'elliptical' to the parameter descriptions in the docstring of CreateMask.

This comment has been minimized.

@stinebuu

stinebuu Mar 16, 2017 Author Contributor

Done! :)


def SelectNodesByMask(layer, anchor, mask_obj):
"""
Obtain GIDs inside a specified area.

This comment has been minimized.

@jhnnsnk

jhnnsnk Mar 15, 2017 Contributor

Please make the description a bit more detailed, i.e., point out that GIDs are obtained inside one topology layer only and that the area is specified by a mask.

Returns
-------
out : list of int(s)
GID(s) of neurons inside the mask.

This comment has been minimized.

@jhnnsnk

jhnnsnk Mar 15, 2017 Contributor

Does it work only for neurons or also for other nodes/elements inside the mask?


self.assertEqual(gid_list, (13, 18, 23,))

def test_EllpsoidalMask3D(self):

This comment has been minimized.

@jhnnsnk

jhnnsnk Mar 15, 2017 Contributor

Ellpsoidal -> Ellipsoidal


self.assertTrue(mask.Inside([0.0, 0.0]))

def test_EllpticalMask2D(self):

This comment has been minimized.

@jhnnsnk

jhnnsnk Mar 15, 2017 Contributor

Ellptical -> Elliptical

Copy link
Contributor

jhnnsnk left a comment

It looks good to me now, thanks!

@heplesser heplesser merged commit 72040a5 into nest:master Mar 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hakonsbm hakonsbm deleted the hakonsbm:app_branch branch Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.