Skip to content

[bug fix #255] found bug#257

Merged
Didou09 merged 6 commits intofeature-#252from
bf-#255
Nov 19, 2019
Merged

[bug fix #255] found bug#257
Didou09 merged 6 commits intofeature-#252from
bf-#255

Conversation

@lasofivec
Copy link
Copy Markdown
Collaborator

Fixes #255

@lasofivec lasofivec requested a review from Didou09 November 18, 2019 17:07
@lasofivec lasofivec self-assigned this Nov 18, 2019
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Nov 19, 2019

Hello @lasofivec! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 158:39: E713 test for membership should be 'not in'

Comment last updated at 2019-11-19 17:33:20 UTC

Copy link
Copy Markdown
Member

@Didou09 Didou09 left a comment

Choose a reason for hiding this comment

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

Good, but:

  • Could you find the reason why an error was thrown with vect=None ? It should not
  • ani=None should remain the default, None is neutral and means the user did not specify anything. False / True are choices that the user wants to enforce.

Comment thread tofu/geom/_core.py Outdated
Comment thread tofu/geom/_core.py
@lasofivec
Copy link
Copy Markdown
Collaborator Author

Good, but:

  • Could you find the reason why an error was thrown with vect=None ? It should not

Please see the lines where I tagged you

Comment thread tofu/geom/_GG.pyx
ray_vdir[:, ii:ii+1])
val_2d = func(pts, t=t, vect=-usbis, **fkwdargs)
sig_mv[:, ii] = np.sum(val_2d, axis=-1)*loc_eff_res[0]
sig[:, ii] = np.sum(val_2d, axis=-1)*loc_eff_res[0]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Didou09 :

Could you find the reason why an error was thrown with vect=None ? It should not

It doesn't anymore. Here is the only real change in this PR, that fixes the bug.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

PS: if you see line 3319, it only happened if ani=True, minimize=hybrid, method=sum, and if you see line 2863 I took out the variable which was no longer need and verified all loops to see that this error was nowhere else.

Comment thread tofu/tests/tests09_tutorials/tests01_runall.py
@codecov-io
Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (feature-#252@cf892fe). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##             feature-#252     #257   +/-   ##
===============================================
  Coverage                ?   43.91%           
===============================================
  Files                   ?       74           
  Lines                   ?    21518           
  Branches                ?        0           
===============================================
  Hits                    ?     9449           
  Misses                  ?    12069           
  Partials                ?        0
Impacted Files Coverage Δ
tofu/geom/_core.py 63.5% <ø> (ø)
tofu/tests/tests09_tutorials/tests01_runall.py 65.59% <66.66%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf892fe...8ab1c43. Read the comment docs.

@Didou09 Didou09 merged commit 8b88511 into feature-#252 Nov 19, 2019
@Didou09 Didou09 deleted the bf-#255 branch November 19, 2019 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants