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

Allow rectangular and box masks to be rotated #845

Merged
merged 10 commits into from Mar 7, 2018

Conversation

Projects
None yet
4 participants
@stinebuu
Contributor

stinebuu commented Oct 25, 2017

This PR adds the optional parameters azimuth_angle and polar_angle to the rectangular and box mask dictionaries so that the masks can be rotated.

This have led to some changes in the boxMask class, most notably with the inside(..) function and when finding the boundary box. When checking if a point is inside the mask, we have to take rotation into account. If we have rotation, we can no longer use lower_left and upper_right to define the boundary box, so we have to calculate the minimum and maximum values of the mask. This is done in create_min_max_values_().

The functions check to see if we have rotation as much as possible, so that we can use lower_left and upper_right and avoid unnecessary rotation calculations if we do not have rotation.

I have also added a test.

@stinebuu stinebuu changed the title from Allow rectangle and box masks to be rotated to Allow rectangular and box masks to be rotated Oct 25, 2017

@heplesser heplesser requested review from heplesser and hakonsbm Oct 30, 2017

@hakonsbm

Thank you for this, it looks good. As far as I can see, there are just some minor things to fix, see below.

Show outdated Hide outdated topology/mask.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/pynest/tests/test_rotated_rect_mask.py
Show outdated Hide outdated topology/pynest/tests/test_rotated_rect_mask.py
Show outdated Hide outdated topology/pynest/tests/test_rotated_rect_mask.py
Show outdated Hide outdated topology/pynest/tests/test_rotated_rect_mask.py
def suite():
suite = unittest.makeSuite(RotatedRectangularMask, 'test')
return suite

This comment has been minimized.

@hakonsbm

hakonsbm Oct 31, 2017

Contributor

Two blank lines after the function definition.

@hakonsbm

hakonsbm Oct 31, 2017

Contributor

Two blank lines after the function definition.

stinebuu added some commits Nov 1, 2017

Made requested changes; create_min_max_values now called
calculate_min_max_values, precalculation of some of the values,
indentation, changed some comments.
@hakonsbm

Great, thank you!

@heplesser

Looks quite fine, but see comments. We should discuss once more about the "eps" that we are adding, just wondering if we are digging some future grave. For 3D, we olny have two angles. Which rotation possibility are we missing?

Show outdated Hide outdated topology/mask.h
* upper_right - Position of upper right corner (array of doubles)
* lower_left - Position of lower left corner (array of doubles)
* upper_right - Position of upper right corner (array of doubles)
* azimuth_angle - Rotation angle in degrees from x-axis (double), optional

This comment has been minimized.

@heplesser

heplesser Nov 30, 2017

Contributor

Is it documented somewhere that the angles are in degrees? Maybe also add that the polar angle does not apply to 2D.

@heplesser

heplesser Nov 30, 2017

Contributor

Is it documented somewhere that the angles are in degrees? Maybe also add that the polar angle does not apply to 2D.

This comment has been minimized.

@stinebuu

stinebuu Dec 8, 2017

Contributor

I have now added a note about the angles in the python documentation for masks.

@stinebuu

stinebuu Dec 8, 2017

Contributor

I have now added a note about the angles in the python documentation for masks.

Show outdated Hide outdated topology/mask.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Show outdated Hide outdated topology/mask_impl.h
Added some documentation, values in inside function not dependent on
position is pre-calculated in constructor, added some const, eps no
longer calculated from length, but just a constant, small number.
@stinebuu

This comment has been minimized.

Show comment
Hide comment
@stinebuu

stinebuu Dec 8, 2017

Contributor

@heplesser thank you for your review! I have tried to address your comments, please take a look.

I changed the "eps" to be constant 1e-12 for x, y and z. Having it depend on the center of the box will fail when the center is in the origin, as eps then will be zero.

When it comes to the rotation angles, we are missing rotation from the y-axis to the z-axis (the γ here ). Do you want me to add it? We do not have it for the elliptical mask, but I could add it there as well and make a PR of course.

Contributor

stinebuu commented Dec 8, 2017

@heplesser thank you for your review! I have tried to address your comments, please take a look.

I changed the "eps" to be constant 1e-12 for x, y and z. Having it depend on the center of the box will fail when the center is in the origin, as eps then will be zero.

When it comes to the rotation angles, we are missing rotation from the y-axis to the z-axis (the γ here ). Do you want me to add it? We do not have it for the elliptical mask, but I could add it there as well and make a PR of course.

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 7, 2018

Contributor

@stinebuu Thanks for addressing the issues. I think this ready to merge now. Would you create a follow-up issue to add rotation about the third Euler angle?

Contributor

heplesser commented Mar 7, 2018

@stinebuu Thanks for addressing the issues. I think this ready to merge now. Would you create a follow-up issue to add rotation about the third Euler angle?

@heplesser heplesser merged commit c11bb2c into nest:master Mar 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stinebuu stinebuu deleted the stinebuu:tilted_rectangle_mask branch Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment