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

Adding flatmap rendering functionallity #269

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Tal-Golan
Copy link

@Tal-Golan Tal-Golan commented Jun 24, 2019

Addressing issue 214, I added flat-map rendering functionality. This was done by creating a Patch, a child object of Surface, that deals with storing the relevant meta-data (which patch vertices correspond to which surface vertices). I worked over all of the examples and modified the relevant functions so they work with flatmaps as seamlessly as possible.
image

Copy link
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising. Missing tests of this new functionality, can you add them?

Circle fails:

https://circleci.com/gh/nipy/PySurfer/253

I can add the patch files to the archive used here in a separate PR. Is some specific FreeSurfer version required?

configure_input_data(mapper, mesh.data)
actor = tvtk.Actor()
actor.mapper = mapper
fig.scene.add_actor(actor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not modify every example, just one or two. Maybe adding the plot_flatmap.py is already enough.

cam = fig.scene.camera
cam.zoom(1.85)

mlab.savefig('test.png',figure=fig,magnification=5) # save a high-res figure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this savefig command in the example


brain.show_view("medial")
if not hasattr(brain,'patch_mode'):
brain.show_view("medial")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove this

@@ -34,4 +34,5 @@
line_width=3, hemi=hemi)
brain.contour_list[-1]["colorbar"].visible = False

brain.show_view("dorsal")
if not brain.patch_mode:
brain.show_view("dorsal")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of forcing people to write conditionals, in brain.show_view I would check to see if in patch_mode and if so, discard the azimuth/elevation/roll arguments (only pay attention to distance), and say that this is the behavior in the show_view docstring. If people really want to configure the view they can use mlab to do it.

if 'patch' in surf.lower().split('.'):
view=None
else:
view = 'frontal'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these conditionals are needed with the show_view modification suggested above.


# reverse the lookup table:
original_vertices_in_patch_indexing=np.zeros((n_orig_vertices,)); original_vertices_in_patch_indexing[:]=np.nan
original_vertices_in_patch_indexing[patch_vertices_in_original_surf_indexing]=np.arange(len(patch_vertices_in_original_surf_indexing))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many PEP8 violations in this and other files, can you fix them?

for v in args:
cut_v.append(np.asarray(v)[vertices_in_patch])
return (vertices,)+tuple(cut_v)
def read_patch_file(fname):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this in nibabel? If not, can you make a PR there, too?

surface where hemispheres typically overlap (Default: True)
surface where hemispheres typically overlap. For flat patches,
this aligns the two patches along their posterior
edges. (Default: True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also update the surface argument above to say that it's supported, and add a:


.. versionchanged:: 0.10
   Support for flat maps.

@larsoner larsoner mentioned this pull request Jun 25, 2019
@larsoner
Copy link
Contributor

Now that #270 is in, if you rebase CircleCI might render properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants