Start year flexibility

[RhysMacMillan]

This PR adds the ability for users to choose their own first year for data frame columns. The year 1965 occurs as an arbitrary start year throughout the scripts folder, and this change allows more flexibility for the user.

Edit: This PR addresses part of issue #60, specifically the hard coded variables in dataframe_analysis_changes

[pep8speaks]

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-08-13 19:40:04 UTC

[nsryan2]

Great start @RhysMacMillan, a couple of suggestions to help improve this PR! I like that you've given the year a variable, and by making the default value the same as the previous you avoided needing to alter the tests (smart).

1. Totally cool to start with updating the year assumption in the scripts, I think we can make this PR a little bit bigger. Either find the rest of the 1965 assumptions, or change the rest of the hard coded variables you've identified in dataframe_analysis.
2. In the ARFC repos we have a bot that comes in and checks for style in Python code. It's based on PEP 8 (Python Enhancement Proposal number 8, which is the official style guide for python that we use). Whenever we make a PR, the bot will review the code changes to ensure the resulting files still comply with the style guide. In this case, the bot has identified two places where you've mixed spaces and tabs. Here's what PEP8 says should be done with indentation; check it out, and push some commits from your clone to your fork.
3. I suggested expanding your variable s_y to start_year. I could be convinced that y0 is better, but I think that s_y doesn't convey enough information to people. This is totally a preference thing, and is kind of nitpicky. On my suggestions, you'll see the option to Add to batch, which you can do and then commit them all at once, or you can change it to y0 if you like that better and ignore my suggestions!
4. When there is an associated issue with your PR, you should always reference that PR in your comment. If your PR totally addresses the issue, than you can use a keyword (I think I've sent you this link before) to tell GitHub to close the Issue when the PR is merged. In this case, you are not totally addressing the issue, so you should say something to the effect of "This PR addresses part of # 60" (don't put the space after the #, I did that so my comment wouldn't link the PR and the Issue).

[RhysMacMillan]

Thank you for the feedback @nsryan2. I couldn't decide between y0 and s-y but I do like y0 more than start_year because of its brevity. I would like to change this variable name, the other assumptions in dataframe_analysis and the PEP 8 style issue in one action, so would making a new PR referencing this one be the best course of action?

[nsryan2]

Ok! Yeah, I think y0 is better than s_y.

You can make changes by making changes on your clone and then pushing them to the dataframe_analysis_changes branch you made this PR from on your fork. You don't need to make a new PR.

Edit: I have updated my suggestions to say y0 instead of start_year. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

[katyhuff]

This looks fine to me.

[nsryan2]

@RhysMacMillan are you still adding changes to this PR? You mentioned some other assumptions above but I'm not sure if you've done all the ones you wanted to do. Otherwise, it looks good to me

[RhysMacMillan]

Yes @nsryan2 I have one last change for the start year assumption. In create_cyclus_input.py there's a global variable; start_year = 1965 and I'm not sure how to address this change. My first thought was to have a user input but I don't know how the scripts are interacted with so I didn't want to make it cumbersome.

[nsryan2]

This would be a great thing to mention in #162! If you make a note of that change as a comment in that issue, I think that would be good. If that's the case, I'll merge this PR. Thanks for the contribution!