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

Maybe there is a bug in code of Array Operations with NumPy #60

Closed
fidle opened this issue Apr 5, 2019 · 3 comments · Fixed by #61
Closed

Maybe there is a bug in code of Array Operations with NumPy #60

fidle opened this issue Apr 5, 2019 · 3 comments · Fixed by #61
Labels

Comments

@fidle
Copy link

fidle commented Apr 5, 2019

In . There is a little bug in sentence:
u[1:, 1:] = un[1:, 1:] - ((c * dt / dx * (un[1:, 1:] - un[1:, 0:-1])) -
(c * dt / dy * (un[1:, 1:] - un[0:-1, 1:])))

The bold left bracket "(" is in a wrong place. It should follow "=" . Correct code should be:
u[1:, 1:] = ( un[1:, 1:] -(c * dt / dx * (un[1:, 1:] - un[1:, 0:-1])) -
(c * dt / dy * (un[1:, 1:] - un[0:-1, 1:])))
This little bug make cell5 and cell6 show a different result which suppose to be same.

@mesnardo
Copy link
Member

mesnardo commented Apr 5, 2019

Thank you for opening this issue @fidle.
Correct me if I am wrong, but I think you are referring to In [5] and In [6] in Lesson 06 (06_Array_Operations_with_NumPy.ipynb).
If so, the parenthesis in In [6] are indeed misplaced.

Here is a small example that reflects this issue:

> python
Python 3.6.8 |Anaconda custom (64-bit)| (default, Dec 29 2018, 19:04:46)
[GCC 4.2.1 Compatible Clang 4.0.1 (tags/RELEASE_401/final)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy
>>> numpy.__version__
'1.15.4'
>>> c = 1.0
>>> dx = 2 / 80
>>> dy = 2 / 80
>>> sigma = 0.2
>>> dt = sigma * dx
>>> un = numpy.random.rand(80, 80)
>>> u1 = un[1:, 1:] - ((c * dt / dx * (un[1:, 1:] - un[1:, 0:-1])) - (c * dt / dy * (un[1:, 1:] - un[0:-1, 1:])))
>>> u2 = (un[1:, 1:] - (c * dt / dx * (un[1:, 1:] - un[1:, 0:-1])) - (c * dt / dy * (un[1:, 1:] - un[0:-1, 1:])))
>>> numpy.allclose(u1, u2)
False
>>>

Since you found the bug @fidle, would you like to provide a patch in a pull-request?

@mesnardo mesnardo added the bug label Apr 5, 2019
@fidle
Copy link
Author

fidle commented Apr 7, 2019

sorry to reply you so late. @mesnardo
you are right , the parenthesis in In[6] is misplace. I am sorry I did make it clear in the issue.
I hope I could make a pull-request. but I don't know how(I am a fresh man in github).
I rewrite this part to make the code output a result, so people can see that both method can get the same result.
here is the code
`import numpy
from matplotlib import pyplot

nx = 81
ny = 81
dx = 2/(nx-1)
dy = 2/(ny-1)
c = 1
nt = 100
sigma =0.2
dt = sigma*dx

x = numpy.linspace(0,2,nx)
y = numpy.linspace(0,2,ny)
%%timeit
u = numpy.ones((ny, nx))
u[int(.5 / dy): int(1 / dy + 1), int(.5 / dx):int(1 / dx + 1)] = 2

for n in range(nt + 1): ##loop across number of time steps
un = u.copy()
row, col = u.shape
for j in range(1, row):
for i in range(1, col):
u[j, i] = (un[j, i] - (c * dt / dx *
(un[j, i] - un[j, i - 1])) -
(c * dt / dy *
(un[j, i] - un[j - 1, i])))
u[0, :] = 1
u[-1, :] = 1
u[:, 0] = 1
u[:, -1] = 1
pyplot.pcolormesh(u)`

`%%timeit
u = numpy.ones((ny, nx))
u[int(.5 / dy): int(1 / dy + 1), int(.5 / dx):int(1 / dx + 1)] = 2

for n in range(nt + 1): ##loop across number of time steps
un = u.copy()
u[1:, 1:] = un[1:, 1:] - (c * dt / dx * (un[1:, 1:] - un[1:, 0:-1]))-(c * dt / dy * (un[1:, 1:] - un[0:-1, 1:]))
u[0, :] = 1
u[-1, :] = 1
u[:, 0] = 1
u[:, -1] = 1
pyplot.pcolormesh(u)`

mesnardo added a commit to mesnardo/CFDPython that referenced this issue Apr 8, 2019
Fix barbagroup#60

The parenthesis in In [6] were miscplaced.
@fidle caught the bug and opened an issue (barbagroup#60).
Thank you for reporting the error @fidle.

Also:

* Re-run notebook to get updated execution times.
* Update execution times in Markdown cells.
* Add info about machine used to re-run notebook.
@mesnardo
Copy link
Member

mesnardo commented Apr 8, 2019

Thank you @fidle.
I opened a pull-request to correct the parenthesis.
Here are the steps I did to submit a pull-request:

  • I forked the repository barbagroup/CFDPython to my GitHub account.
  • On my local machine, I cloned the forked repository mesnardo/CFDPython.
  • I created a new branch called misplaced-parenthesis from the master branch.
  • I corrected the Notebook (based on your report) and re-ran the entire Notebook.
  • I committed and pushed to mesnardo/CFDPython to the branch misplaced-parenthesis.
  • On GitHub, I created a pull-request to request a merge from my branch misplaced-parenthesis to the master branch of barbagroup/CFDPython (see PR [L6] Correct misplaced parenthesis #61).
  • I am now waiting for one of the maintainers to review and approve my pull-request and to finalize the merge.

(@fidle, I gave you credit in the commit message 3c692c4.)

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

Successfully merging a pull request may close this issue.

2 participants