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

Zernike refactor #42

Closed
brianthelion opened this issue Mar 19, 2014 · 17 comments
Closed

Zernike refactor #42

brianthelion opened this issue Mar 19, 2014 · 17 comments
Assignees

Comments

@brianthelion
Copy link
Contributor

  1. Legacy code should be moved into the test suite.
  2. Test suite should be updated.
  3. File proliferation should be reduced or eliminated.
  4. Code should be PEP8 compliant.
  5. Remaining MATLAB idioms should be replaced with Numpy idioms.

Anything else?

@brianthelion brianthelion self-assigned this Mar 19, 2014
@binarybottle
Copy link
Member

Those are all good. I would add documentation of the code so that it will be easier to maintain in the future.

@brianthelion
Copy link
Contributor Author

Making incremental progress here, a few hours a day. Expect a push this week.

@binarybottle
Copy link
Member

Great!

Cheers,
@rno

On Mon, Apr 28, 2014 at 4:05 PM, brianthelion [email protected]:

Making incremental progress here, a few hours a day. Expect a push this
week.


Reply to this email directly or view it on GitHubhttps://github.com//issues/42#issuecomment-41625205
.

@brianthelion
Copy link
Contributor Author

Didn't get quite as much done as I had hoped last week. Picking it up again tomorrow and will update on progress then. Cheers!

@binarybottle
Copy link
Member

Great -- Looking forward to your update!

Cheers,
@rno

On Mon, May 5, 2014 at 12:54 PM, brianthelion [email protected]:

Didn't get quite as much done as I had hoped last week. Picking it up
again tomorrow and will update on progress then. Cheers!


Reply to this email directly or view it on GitHubhttps://github.com//issues/42#issuecomment-42230861
.

@brianthelion
Copy link
Contributor Author

Documentation is still to-do.

brianthelion pushed a commit that referenced this issue May 13, 2014
@binarybottle
Copy link
Member

Good show, Brian!
After you implemented Koehl's exact method, it became significantly faster. It took only 10 minutes for order 10 for two cortical hemispheres!

@brianthelion
Copy link
Contributor Author

The Koehl implementation is covered in #45. There are still a few things to do to wrap this ticket, like pulling together the documentation.

@brianthelion brianthelion reopened this Jun 25, 2014
binarybottle pushed a commit that referenced this issue Oct 4, 2014
binarybottle pushed a commit that referenced this issue Oct 4, 2014
binarybottle pushed a commit that referenced this issue Oct 4, 2014
binarybottle pushed a commit that referenced this issue Oct 4, 2014
@brianthelion
Copy link
Contributor Author

@binarybottle: I finally have some time to put on this cleanup task. Unfortunately I haven't tracked our priorities w.r.t tasks 1-5 listed above. Is documentation the only thing that remains to be done at this time? Cheers!

@binarybottle
Copy link
Member

I believe documentation was the only thing left to do, but after Satra found that a 2GB memory limit was exceeded by computing Zernike moments, this strikes me as extremely important to address!

From Satra:
150215-15:17:45,874 workflow INFO:
Executing node Zernike_sulci.a1 in dir: /om/scratch/Tue/ps/MB_work/734db8e05f6be469df79c1419f253ad7/Mindboggle/Surface_feature_shapes/_hemi_rh/Zernike_sulci
Load "sulci" scalars from sulci.vtk
8329 vertices for label 1
Reduced 160076 to 15921 triangular faces
srun: Exceeded job memory limit

@brianthelion
Copy link
Contributor Author

Ok, see my comments on #52.

@brianthelion
Copy link
Contributor Author

@binarybottle Is there a particular documentation standard that you're sticking with on Mindboggle?

@binarybottle
Copy link
Member

If you take a look at any of the other scripts, you'll see that I use restructured text. I use Sphinx to create docs from the code. Does that answer your question?

@brianthelion
Copy link
Contributor Author

@binarybottle Yep! Thanks.

@binarybottle
Copy link
Member

@brianthelion -- If you are unable to devote any more time to documentation of the code, shall I close this issue?

@satra
Copy link
Member

satra commented Mar 10, 2016

@binarybottle - was this fixed?

@binarybottle
Copy link
Member

Since the only lingering part of the discussion was the question of documentation, I didn't think it helpful to keep this issue open.

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

No branches or pull requests

3 participants