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

Fix evolution save win32 #469

Merged
merged 2 commits into from
Nov 19, 2014

Conversation

Garyfallidis
Copy link
Contributor

This should resolve issue #467.

@@ -61,6 +61,7 @@ def test_rigid_parallel_lines():
bundle_initial = simulated_bundle()
bundle, shift = center_streamlines(bundle_initial)
mat = compose_matrix44([20, 0, 10, 0, 40, 0])

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a problem for this PR, but for my taste, and for PEP8 - there are lots of extra blank lines. You monitor may be bigger than mine, and I bet your eyesight is better and fontsize smaller, but the extra whitespace makes the code harder for me to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay true. I have to remove some spaces. But because PEP8 is good you can merge this PR. I am working right now on another small extension PR for the Streamline-based Registration and I can remove unneeded spaces in this one. I hope that this sounds good for you. Let me know otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sure, no need to hold up the PR for the blank lines, it was just something I noticed.

PEP8 says 'Use blank lines in functions, sparingly, to indicate logical sections.'.

I know it's a matter of taste, and I'm not generally a huge PEP8 person, but lots of blank lines does look distracting to me, makes it harder to follow the flow.

@matthew-brett
Copy link
Contributor

Looks good to me.

matthew-brett added a commit that referenced this pull request Nov 19, 2014
MRG: Fix evolution save win32

Use lists instead of files to track progress of optimizer.

This should resolve issue #467.
@matthew-brett matthew-brett merged commit 03718f5 into dipy:master Nov 19, 2014
@Garyfallidis
Copy link
Contributor Author

I agree completely with what you say. Blank lines should be used to separate logical regions. It helps a lot. Will do the change for sure :) Thx again for the feedback. But please also tell me when you find some time what is a safe way to save tmp files during object deletion.

@matthew-brett
Copy link
Contributor

You just have to make sure that you have delete any objects that could possibly be holding onto these file objects before you delete the files, in Windows. It can be difficult to tell though.

In practice, I end up logging into a Windows machine and futzing with deleting objects (del my_object) before deleting the file, until I find the one that's holding onto it.

We also have the fab try_branch.py approach that Omar and I developed, where you can submit changes on a branch direct to a specific (set of) buildbots. If you want to know how to do that, let me know.

@Garyfallidis
Copy link
Contributor Author

Yes, I would love to know about try_branch.py

@matthew-brett
Copy link
Contributor

@arokem
Copy link
Contributor

arokem commented Nov 19, 2014

Super-useful! Thanks for doing that.

On Wed, Nov 19, 2014 at 11:36 AM, Matthew Brett notifications@github.com
wrote:

Wrote up instructions here :
https://github.com/nipy/nibotmi/blob/master/install.rst#trying-a-set-of-changes-on-the-buildbots


Reply to this email directly or view it on GitHub
#469 (comment).

@matthew-brett
Copy link
Contributor

De nada - let me know if it doesn't work right - the whole try branch mechanism is a bit delicate in buildbot.

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.

3 participants