search

arfc / transition-scenarios

A repository to hold transition scenarios with Cyclus.
Other
3 stars 12 forks source link

Modifies functions and tests for analysis #125

Closed abachma2 closed 2 years ago

abachma2 commented 2 years ago

This PR includes changes to the files scripts/transition_metrics.py and scripts/tests/test_transition_metrics.py. The modifications to the functions make them a little bit easier to use, simplify their operations, or add new capabilities. The tests were modified to pass based on changes to the output files or test new capabilities of new functions. One of the output databases was changed to ensure the problem definition given at the beginning of the class was met.

pep8speaks commented 2 years ago

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

Line 169:80: E501 line too long (82 > 79 characters) Line 170:80: E501 line too long (83 > 79 characters) Line 189:80: E501 line too long (82 > 79 characters) Line 190:80: E501 line too long (83 > 79 characters)

Line 179:80: E501 line too long (87 > 79 characters)

Comment last updated at 2022-03-07 22:27:08 UTC
abachma2 commented 2 years ago

I have made most of the requested changes. I did not change the name of the test functions because I have the doc strings to accompany each test, and function names would be very long to have descriptive names to describe the corner cases some of them test.

yardasol commented 2 years ago

I have made most of the requested changes. I took a look at your changes and they really improve the readability of the code. Great job!

I did not change the name of the test functions because I have the doc strings to accompany each test, and function names would be very long to have descriptive names to describe the corner cases some of them test.

That's totally fair, and when I wrote my initial review I wasn't considering the fact that may of these were corner cases.