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

[PullRequest.com] Project review #639

Open
wants to merge 1 commit into
base: ​removed_code_for_pullrequest
Choose a base branch
from

Conversation

PullRequest-Agent
Copy link

@PullRequest-Agent PullRequest-Agent commented Dec 16, 2020

PullRequest Review

December 2020

This review analysis of the Paramak project was completed by a team of professional engineers (reviewers) from the PullRequest network and sponsored by the UK Atomic Energy Authority (UKAEA).

PullRequest's review of the repository was conducted with objectives for evaluating the project as a collective whole. In addition to a higher-level assessment report (delivered separately), specific issues are noted and suggestions for improvements offered based on what the reviewing team deemed to be of high utility for the maintainers.

These code-level, inline comments include recommendations and issues cited where location references such as file names, line numbers, and surrounding context were deemed by engineers in the PullRequest reviewer network to be useful.

NOTE: This pull request is not a proposal to merge code. This pull request was created as a vehicle to deliver findings as part of the larger, aforementioned project assessment.

For questions, please contact info@pullrequest.com

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (​removed_code_for_pullrequest@0a3ddbf). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                       Coverage Diff                        @@
##             ​removed_code_for_pullrequest     #639   +/-   ##
================================================================
  Coverage                                 ?   96.33%           
================================================================
  Files                                    ?       69           
  Lines                                    ?     4453           
  Branches                                 ?        0           
================================================================
  Hits                                     ?     4290           
  Misses                                   ?      163           
  Partials                                 ?        0           

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 0a3ddbf...0d432fa. Read the comment docs.

Dockerfile Show resolved Hide resolved
build.sh Show resolved Hide resolved
@@ -0,0 +1,213 @@
# This dockerfile can be built in a few different ways.
Copy link
Author

Choose a reason for hiding this comment

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

BUG RISK
Currently, the project is installed in /. This is fine, but it clutters the root directory of the docker image and makes it so that the user cannot simply volume mount their current directory to test its changes via:

docker run -ti --rm -v `pwd`:/paramak ukaea/paramak bash

If the user tries to volume mount inside of /, then they'll replace the entire filesystem.

# using setup.py instead of pip due to https://github.com/pypa/pip/issues/5816
RUN python setup.py install

WORKDIR examples
Copy link
Author

@PullRequest-Agent PullRequest-Agent Dec 16, 2020

Choose a reason for hiding this comment

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

MAINTAINABILITY
Attempting to run one of the examples inside of the Docker container, this error was encountered:

(base) root@07c6f4d85ce9:/# python examples/example_neutronics_simulations/ball_reactor.py 
/opt/conda/lib/python3.7/site-packages/neutronics_material_maker/utils.py:14: UserWarning: OpenMC not found, .openmc_material, .serpent_material, .mcnp_material,            .fispact_material not avaiable
  .fispact_material not avaiable")
/opt/conda/lib/python3.7/site-packages/neutronics_material_maker/material.py:29: UserWarning: OpenMC python package not found, .openmc_material, .serpent_material,             .mcnp_material, .fispact_material methods not avaiable
  .mcnp_material, .fispact_material methods not avaiable")
/opt/conda/lib/python3.7/site-packages/neutronics_material_maker/mutlimaterial.py:21: UserWarning: OpenMC python package not found, .openmc_material, .serpent_material,             .mcnp_material, .fispact_material methods not avaiable
  .mcnp_material, .fispact_material methods not avaiable")
/opt/conda/lib/python3.7/site-packages/paramak-0.1.0-py3.7.egg/paramak/parametric_neutronics/neutronics_model_from_reactor.py:14: UserWarning: parametric_plasma_source not found distributed plasma             sources are not avaialbe in Neutronics simulations
  sources are not avaialbe in Neutronics simulations', UserWarning)
/opt/conda/lib/python3.7/site-packages/paramak-0.1.0-py3.7.egg/paramak/parametric_neutronics/neutronics_model_from_reactor.py:20: UserWarning: OpenMC not found, NeutronicsModelFromReactor.simulate             method not available
  method not available', UserWarning)
Traceback (most recent call last):
  File "examples/example_neutronics_simulations/ball_reactor.py", line 66, in <module>
    make_model_and_simulate()
  File "examples/example_neutronics_simulations/ball_reactor.py", line 39, in make_model_and_simulate
    temperature_in_K=500),
  File "/opt/conda/lib/python3.7/site-packages/neutronics_material_maker/material.py", line 206, in __init__
    self._populate_from_inbuilt_dictionary()
  File "/opt/conda/lib/python3.7/site-packages/neutronics_material_maker/material.py", line 745, in _populate_from_inbuilt_dictionary
    "enrichment_target"
  File "/opt/conda/lib/python3.7/site-packages/neutronics_material_maker/material.py", line 528, in enrichment_target
    if value not in openmc.data.NATURAL_ABUNDANCE.keys():
NameError: name 'openmc' is not defined

It would appear that one of the projects that paramak depends on (openmc) isn't installed in the Dockerfile or it's not available in the default environment.


When we were able to run the tests after fixing the issue above , we came across a string of warnings:

==================================== warnings summary =====================================
paramak/shape.py:5
  /paramak/shape.py:5: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3,and in 3.9 it will stop working
    from collections import Iterable

paramak/parametric_neutronics/neutronics_model_from_reactor.py:14
  /paramak/parametric_neutronics/neutronics_model_from_reactor.py:14: UserWarning: parametric_plasma_source not found distributed plasma             sources are not avaialbe in Neutronics simulations
    sources are not avaialbe in Neutronics simulations', UserWarning)

paramak/parametric_neutronics/neutronics_model_from_reactor.py:20
  /paramak/parametric_neutronics/neutronics_model_from_reactor.py:20: UserWarning: OpenMC not found, NeutronicsModelFromReactor.simulate             method not available
    method not available', UserWarning)

opt/conda/lib/python3.7/site-packages/neutronics_material_maker/utils.py:14
  /opt/conda/lib/python3.7/site-packages/neutronics_material_maker/utils.py:14: UserWarning: OpenMC not found, .openmc_material, .serpent_material, .mcnp_material,            .fispact_material not avaiable
    .fispact_material not avaiable")

opt/conda/lib/python3.7/site-packages/neutronics_material_maker/material.py:29
  /opt/conda/lib/python3.7/site-packages/neutronics_material_maker/material.py:29: UserWarning: OpenMC python package not found, .openmc_material, .serpent_material,             .mcnp_material, .fispact_material methods not avaiable
    .mcnp_material, .fispact_material methods not avaiable")

opt/conda/lib/python3.7/site-packages/neutronics_material_maker/mutlimaterial.py:21
  /opt/conda/lib/python3.7/site-packages/neutronics_material_maker/mutlimaterial.py:21: UserWarning: OpenMC python package not found, .openmc_material, .serpent_material,             .mcnp_material, .fispact_material methods not avaiable
    .mcnp_material, .fispact_material methods not avaiable")

...

tests/test_example_reactors.py::test_object_properties::test_make_parametric_ball_rector
tests/test_example_reactors.py::test_object_properties::test_make_parametric_single_null_ball_reactor
tests/test_parametric_reactors/test_BallReactor.py::test_BallReactor::test_creation_with_narrow_divertor
tests/test_parametric_reactors/test_BallReactor.py::test_BallReactor::test_divertor_upper_lower
tests/test_parametric_reactors/test_BallReactor.py::test_BallReactor::test_svg_creation
tests/test_parametric_reactors/test_BallReactor.py::test_BallReactor::test_with_pf_and_tf_coils
tests/test_parametric_reactors/test_BallReactor.py::test_BallReactor::test_with_pf_coils
tests/test_parametric_reactors/test_SegmentedBlanketBallReactor.py::test_SegmentedBlanketBallReactor::test_gap_between_blankets_impacts_volume
tests/test_parametric_reactors/test_SegmentedBlanketBallReactor.py::test_SegmentedBlanketBallReactor::test_number_of_blanket_segments_impacts_volume
  /paramak/parametric_components/blanket_fp.py:227: UserWarning:
  
  BlanketFP: Some points with negative R coordinate have been ignored.

tests/test_parametric_components/test_PoloidallySegmentedBlanketFP.py::test_BlanketFP::test_segment_angles_affects_solid
  /paramak/parametric_components/blanket_poloidal_segment.py:61: UserWarning:
  
  start_angle and stop_angle attributes will be ignored if segments_angles is not None

tests/test_parametric_components/test_casing.py::test_TFCoilCasing::test_creation
  /paramak/parametric_components/tf_coil_casing.py:31: UserWarning:
  
  Casing azimuth_placement_angle should be the same value as TFCoilCasing.magnet.

tests/test_parametric_components/test_casing.py::test_TFCoilCasing::test_creation
  /paramak/utils.py:226: RuntimeWarning:
  
  divide by zero encountered in true_divide

-- Docs: https://docs.pytest.org/en/stable/warnings.html
====================== 362 passed, 228 warnings in 402.34s (0:06:42) ======================

There were more than those listed above, shortening for brevity.

It's best practice to ensure minimal warnings when running tests. In this case, there seem to be 228 warnings.

Dockerfile Show resolved Hide resolved
README.md Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@PullRequest-Agent PullRequest-Agent marked this pull request as ready for review December 16, 2020 21:38
@shimwell shimwell mentioned this pull request Dec 21, 2020
10 tasks
@shimwell shimwell mentioned this pull request Feb 9, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants