Conversation
|
Hello @jmoralesFusion! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-11-20 08:38:52 UTC |
|
Thanks @jmoralesFusion for this PR, |
|
The units tests are all passing for python 3.6, which is sufficient because we'll drop python 2.7 from now, so it's fine |
| if t is None: | ||
| t = np.r_[38] | ||
| # Get time in the middle of equilibrium time interval | ||
| t = equi.ddata['equilibrium.t']['data'][int(0.5*equi.ddata['equilibrium.t']['data'].size)] |
Didou09
left a comment
Didou09
left a comment
There was a problem hiding this comment.
See comments, but mostly:
- Please restore the original system path
- please just confirm I have properly understood the changes where commented
And then we'll be all set :-)
| r_init = [equi_r_ext]*nbr_init | ||
| phi_init = [ii*2.*np.pi/nbr_init for ii in range(nbr_init)] | ||
| z_init = [equi_z_r_ext]*nbr_init | ||
| init_plt = [r_init, phi_init, z_init] |
There was a problem hiding this comment.
so if I understand correctly, the starting points are all in the equatorial plane at theta=0 (outer R of separatrix) but shifted toroidally (phi), right ?
| refpt = np.r_[2.4,0.] | ||
| dax = config.plot_phithetaproj_dist(refpt) | ||
|
|
||
| if init is not None: |
There was a problem hiding this comment.
So I understand that this is the user-provided initial points, right ?
| import sys | ||
|
|
||
| #print('path 1 =', sys.path) | ||
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))) |
There was a problem hiding this comment.
Please make sure any change to the system path is exceptional and temporary
The system path should not be changed without the user knowing, unless it is temporary (i.e.: only in for a limited time inside the file)
=> please restore the original system path as soon as possible after the tests are run
=> Actually, this is not necessary, all unit tests should be placed in tofu/tests/
=> Temporary fix: restore original system path in the file, I validate the PR, then we open a new issue to move all unit tests to the proper repo (we will create a dedicated tofu/tests/tests09_mag/ repo, and get the template from other unit tests
| # vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4 | ||
| # Also if needed: retab |
There was a problem hiding this comment.
seems like this is specific to your vim config, and not necessary to add ?
There was a problem hiding this comment.
Agreed, anything specific to a contributor's working environment shall be removed
| #import mag | ||
| from mag import magFieldLines | ||
|
|
||
| doctest.testmod(magFieldLines, verbose=True) |
There was a problem hiding this comment.
As commented in the review, I suggest:
- Step 1: minimize changes to system path and restore original system path before the end of the tests, I think we can validate the PR from there
- Step 2: Move the unit tests to a dedicated repo in tofu/tests (create a dedicated issue and PR)
- Step 3: move from nose to something else (see issue Moving on from nosetests... #232, but we are not there yet)
| parser.add_argument('-t0', '--t0', type=str, required=False, | ||
| help='Reference time event setting t = 0', default=_T0) | ||
| parser.add_argument('-t', '--t', type=float, required=False, | ||
| help='Input time when needed') |
There was a problem hiding this comment.
@Didou09 seems like replacing renaming a flag that you used to be used for something else, and adding a new one (type) for the old one (tokamak), can lead to confusion. Could the long flag for the time could be at least --time, or even better --input-time?
There was a problem hiding this comment.
Discussed together, name flag -tok (short version) is actually probably better for --tokamak and -t for time is also more intuitive.
So it's fine for me.
The question on the long version (i.e.: --t vs --time) remains, I'm not sure what to think, because updating to --time would also require updating the corresponding keyword argument of the python function.
I suggest : leave it as is (--t) for now, and we'll see later if users mention any issue / ambiguity with that
Codecov Report
@@ Coverage Diff @@
## devel #226 +/- ##
=========================================
- Coverage 43.91% 43.7% -0.21%
=========================================
Files 74 75 +1
Lines 21523 21624 +101
=========================================
Hits 9451 9451
- Misses 12072 12173 +101
Continue to review full report at Codecov.
|
| parser.add_argument('-t0', '--t0', type=str, required=False, | ||
| help='Reference time event setting t = 0', default=_T0) | ||
| parser.add_argument('-t', '--t', type=float, required=False, | ||
| help='Input time when needed') |
…cing graph and plot_dist
…t not the pitch angle issue => Jorge
…lotting a manual initial point
|
Hello pep8 now ok except for test files but they will be removed for a test folder when test package will be chosen. And for the _core.py file I didn't touch it so pep8speaks bug? |
|
Fixes in devel Issue #235 |
6 participants
It will not pass pep8 sorry I will modify next week