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

Fixes #1061 : Changed all S0 to 1.0 #1063

Merged
merged 4 commits into from Jul 15, 2016

Conversation

Projects
None yet
4 participants
@sahmed95
Contributor

sahmed95 commented May 27, 2016

There were specific places where S0=123. which I changed to 100.

@sahmed95 sahmed95 changed the title from Fixed #1061 : Changed all S0 to 100.0 to Fixes #1061 : Changed all S0 to 100.0 May 27, 2016

@arokem

This comment has been minimized.

Member

arokem commented May 27, 2016

I think we want to leave all the tests as they were. We want to make sure that things work for arbitrary inputs. For example, if a user decides they want to use S0=123 for some reason, it should still work.

Does that make sense?

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented May 27, 2016

Yes. I think so. That's why I pointed it out as I wasn't sure whether to change the tests. I will keep the tests as they were and update the PR.

Thanks

@sahmed95 sahmed95 force-pushed the sahmed95:issue-1061 branch from 1c37494 to 65ed00f May 27, 2016

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 2, 2016

@i guess I should break this down into multiple commits ? Makes it easier to review and pinpoint where the S0 values should be changed.

Or multiple PR's ? @arokem

@arokem

This comment has been minimized.

Member

arokem commented Jun 2, 2016

No need for multiple PRs or more commits. This is fine. But there is a test failure that needs to be addressed.

@sahmed95 sahmed95 closed this Jun 3, 2016

@sahmed95 sahmed95 force-pushed the sahmed95:issue-1061 branch from 65ed00f to 3dd9e27 Jun 3, 2016

Shahnawaz Ahmed
@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 3, 2016

Ah, @arokem I made by pushing without commiting

@sahmed95 sahmed95 reopened this Jun 3, 2016

@arokem

This comment has been minimized.

Member

arokem commented Jun 3, 2016

@sahmed95 : did you mean to close this PR? It was going in the right direction, and I think we should keep going here, unless you have an alternative plan.

@arokem

This comment has been minimized.

Member

arokem commented Jun 3, 2016

Oh - cool. I see you reopened this right as I sent my message. All good.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 3, 2016

Sorry, that was accidental. I had rebased with master and started fresh and forgot to commit the changes before a forced push. I have made the changes and tests pass locally. There is one place though which still has S0 = 1. (https://github.com/nipy/dipy/blob/master/dipy/reconst/csdeconv.py#L183)

Changing that fails the test_csdeconv. Needs more thought for changing it here I think.
@arokem

@arokem

This comment has been minimized.

Member

arokem commented Jun 28, 2016

Any chance to revisit this one, and complete it?

@sahmed95 sahmed95 changed the title from Fixes #1061 : Changed all S0 to 100.0 to Fixes #1061 : Changed all S0 to 1.0 Jun 28, 2016

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 28, 2016

Hi, seems like S0 = 1. is the preferred choice in most many tests and documentation hence I am setting S0 as 1.

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Changes Unknown when pulling 16dc0de on sahmed95:issue-1061 into * on nipy:master*.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 28, 2016

Current coverage is 80.38%

Merging #1063 into master will decrease coverage by 0.46%

@@             master      #1063   diff @@
==========================================
  Files           200        200          
  Lines         23023      23023          
  Methods           0          0          
  Messages          0          0          
  Branches       2309       2309          
==========================================
- Hits          18614      18508   -106   
- Misses         3933       4035   +102   
- Partials        476        480     +4   

Powered by Codecov. Last updated by fbad873...df1d8fb

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Changes Unknown when pulling 16dc0de on sahmed95:issue-1061 into * on nipy:master*.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jun 29, 2016

This is up for review.

@@ -108,7 +108,7 @@ def __init__(self,
>>> data, gtab = dsi_voxels()
>>> sphere = get_sphere('symmetric724')
>>> from dipy.sims.voxel import SticksAndBall
>>> data, golden_directions = SticksAndBall(gtab, d=0.0015, S0=1, angles=[(0, 0), (90, 0)], fractions=[50, 50], snr=None)
>>> data, golden_directions = SticksAndBall(gtab, d=0.0015, S0=100, angles=[(0, 0), (90, 0)], fractions=[50, 50], snr=None)

This comment has been minimized.

@arokem

arokem Jun 29, 2016

Member

While we're at it, would you mind making this PEP8 compliant, by introducing a line-break?

It should look like this:

data, golden_directions = SticksAndBall(gtab, d=0.0015, S0=100,
... angles=[(0, 0), (90, 0)], fractions=[50, 50], snr=None)

This comment has been minimized.

@arokem

arokem Jun 29, 2016

Member

Trying again:

>>> data, golden_directions = SticksAndBall(gtab, d=0.0015, S0=100,
... angles=[(0, 0), (90, 0)], fractions=[50, 50], snr=None)

This comment has been minimized.

@arokem

arokem Jun 29, 2016

Member

Yes - like my second attempt (hard to indent and align things in this GH textbox...).

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

It needs 3 dots (...) at the beginning of every line in the docstring example. Just like you would get if you ran this in an interactive python session, and put in a line-break there.

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

As in the example I gave in my comment above.

@arokem

This comment has been minimized.

Member

arokem commented Jul 1, 2016

You can run the docstring example tests by running

nosetests --with-doctest 
@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Jul 1, 2016

This should do it. I picked up some more pep8's on the way. I hope that's okay.

Update : Perhaps better to open a new issue for other pep8's

@sahmed95 sahmed95 force-pushed the sahmed95:issue-1061 branch from 15bd0d9 to df1d8fb Jul 1, 2016

@coveralls

This comment has been minimized.

coveralls commented Jul 1, 2016

Coverage Status

Coverage remained the same at 82.908% when pulling df1d8fb on sahmed95:issue-1061 into fbad873 on nipy:master.

@arokem arokem merged commit 330f95b into nipy:master Jul 15, 2016

4 checks passed

codecov/patch 100% of diff hit (target 80.84%)
Details
codecov/project 80.84% (+0.00%) compared to fbad873
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 82.908%
Details
@arokem

This comment has been minimized.

Member

arokem commented Jul 15, 2016

Thanks!

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