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

Avoid Hard-Coded variables #60

Open
katyhuff opened this issue Feb 11, 2018 · 9 comments
Open

Avoid Hard-Coded variables #60

katyhuff opened this issue Feb 11, 2018 · 9 comments

Comments

@katyhuff
Copy link
Member

All scripts in the scripts folder need to be reviewed and to be refactored for clarity.

This issue can be closed when all scripts have been refactored to avoid hard-coding variables without leaving any trace of the source or meaning of the data. No bare numbers, please. Filenames and paths that must be stored may need to be hardcoded, but should be organized and well-named.

@jbae11
Copy link
Collaborator

jbae11 commented Mar 27, 2018

closed by #47

@jbae11 jbae11 closed this as completed Mar 27, 2018
@katyhuff
Copy link
Member Author

#47 was submitted before this issue was created . #47 may well have contributed to the problem. This repository deserves a complete refactor. I am happy to assign an undergrad if you unassign yourself @jbae11 .

@katyhuff katyhuff reopened this Mar 27, 2018
@jbae11
Copy link
Collaborator

jbae11 commented Mar 27, 2018

Oh, I meant #57 I am pretty sure this issue has been addressed and merged as a PR (#57 has some commits that say "meaningful variable names"). But it would be nice to have an undergraduate reseracher take another look!

@katyhuff
Copy link
Member Author

I think that would be best.

@KennellyT KennellyT self-assigned this Jul 3, 2018
@RhysMacMillan RhysMacMillan self-assigned this Aug 7, 2024
@RhysMacMillan
Copy link
Contributor

RhysMacMillan commented Aug 7, 2024

Below are a list of the lines I found that are hard-coded in the scripts folder's files.

analysis.py
924: timestep=10000
1456: width=0.5
1729: 0.00711 - frac_tail
create_AR_DeployInst
600: np.repeat(720, 116)
601: [0] = 600
611: = 960
create_cyclus_input
7: start_year = 1965
33: burns = [33, 51, 100]
dakota_input == n/a
dataframe_analysis
18: np.round(df[‘Time’] / 12 + 1965, 2)
merge_coordinates
156: several > in this nested for loop
output_metrics
281: assays = {hard coded}
324: repeat of above
427: = + 1965
463: range(1965, 2091)
492: same as 427
496: same as 463
predicting_the_past_import
204: several > in this nested for loop
431: if len(start_date) > 4 and float(capacity) > 400
509: this function defaults to 720 months if shutdown date isn’t available
571: if capacity >= 400
random_lifetime_extension == n/a
transition_metrics
260: +1965
288: same as above
320: same as above
325: range(1965, 2091)
355: +1965
359: range(1965, 2091)
transition_plots
444: d = .015 (there is a comment explaining this one)
602: np.arange(0,1450,50)
A whole lot of hard coded elements in this script, but it is for plotting so I imagine a lot of it is just what ended up looking nice.

@nsryan2
Copy link
Member

nsryan2 commented Aug 8, 2024

Great work @RhysMacMillan, I look forward to reviewing you PR here. I think it would be best if you divide it up by script (some of the scripts with a few changes can be grouped together, but try to keep your PRs small so they are easy to review and faster to implement), keep in mind how the changes will affect the tests! When you make a PR, there should be corresponding changes to the tests such that they still work as designed.

@nsryan2
Copy link
Member

nsryan2 commented Aug 8, 2024

If you have ideas or questions about what a variable does and you aren't sure, feel free to ask here so people can refer back to the discussion in the future if they have to.

@RhysMacMillan
Copy link
Contributor

I’ve changed all of the instances of the start year being 1965 except for in create_cyclus_input.py . This was the only global variable instance of 1965 I found and I am not sure how to deal with it.
I considered having a user input for that variable but I don't know how the scripts are interacted with and if this would be cumbersome and unnecessary.

@nsryan2
Copy link
Member

nsryan2 commented Aug 28, 2024

As noted in #162, there's some work on create_cyclus_input.py that needs to be done to make it more user friendly! If you make a note of that assumption in the issue, I think that would be a good resolution for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment