Skip to content

Issue220 new iter config#245

Merged
lasofivec merged 8 commits intodevelfrom
Issue220_NewITERConfig
Nov 5, 2019
Merged

Issue220 new iter config#245
lasofivec merged 8 commits intodevelfrom
Issue220_NewITERConfig

Conversation

@munechika-koyo
Copy link
Copy Markdown
Collaborator

@munechika-koyo munechika-koyo commented Nov 5, 2019

New ITER configuration is added to tofu as a case 'B4' default one.

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Nov 5, 2019 •
edited
Loading

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

Line 645:80: E501 line too long (80 > 79 characters)
Line 646:80: E501 line too long (80 > 79 characters)
Line 647:80: E501 line too long (80 > 79 characters)

Comment last updated at 2019-11-05 16:28:14 UTC

@munechika-koyo munechika-koyo mentioned this pull request Nov 5, 2019
Merged
@munechika-koyo munechika-koyo self-assigned this Nov 5, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 5, 2019 •
edited
Loading

Codecov Report

Merging #245 into devel will increase coverage by 2.46%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            devel     #245      +/-   ##
==========================================
+ Coverage   43.93%   46.39%   +2.46%     
==========================================
  Files          70       70              
  Lines       21288    23493    +2205     
==========================================
+ Hits         9352    10899    +1547     
- Misses      11936    12594     +658
Impacted Files Coverage Δ
tofu/geom/utils.py 43.58% <100%> (ø) ⬆️
tofu/imas2tofu/_core.py 0.66% <0%> (-0.09%) ⬇️
tofu/utils.py 53.97% <0%> (+5.07%) ⬆️
tofu/geom/_core.py 69.05% <0%> (+5.59%) ⬆️

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 fabc36a...48059f4. Read the comment docs.

@lasofivec
Copy link
Copy Markdown
Collaborator

lasofivec commented Nov 5, 2019

Yes !! perfect. I'm going to do a quick push correcting the pep8 speak errors and then it can be merged for me.
I added @Didou09 as a reviewer.

Didou09
Didou09 approved these changes Nov 5, 2019
View reviewed changes
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.

Seems allright to me, thanks for the work !

Comment thread tofu/geom/utils.py
_lok = np.array([_lok, _lok+10])

_here = os.path.abspath(os.path.dirname(__file__))
_root = tofu.__path__[0]
Copy link
Copy Markdown
Member

@Didou09 Didou09 Nov 5, 2019

Choose a reason for hiding this comment

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

good idea !

@Didou09
Copy link
Copy Markdown
Member

Didou09 commented Nov 5, 2019

@lasofivec , are you correcting the last 3 PEP8 non-conformities or do we consider this is ok ?

@lasofivec lasofivec merged commit 488b9f3 into devel Nov 5, 2019
@lasofivec lasofivec deleted the Issue220_NewITERConfig branch November 5, 2019 16:56
@lasofivec
Copy link
Copy Markdown
Collaborator

lasofivec commented Nov 5, 2019

@Didou09, I would say yes :)
(sorry I think we were reviewing the doc at the same time)
lines too long 80 instead of 79, I think that's fine

@Didou09
Copy link
Copy Markdown
Member

Didou09 commented Nov 5, 2019

Nice, congratulations Koyo for your first merged PR !
And thanks Laura for helping a lot !

@Didou09 Didou09 mentioned this pull request Nov 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Didou09 Didou09 Didou09 approved these changes

+1 more reviewer

@lasofivec lasofivec lasofivec approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@munechika-koyo munechika-koyo

Labels

geom - improve

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@munechika-koyo @pep8speaks @codecov-io @lasofivec @Didou09