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

Add nu06 Variable to PUF #329

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Conversation

andersonfrailey
Copy link
Collaborator

This PR changes the nu05 variable to nu06 in puf.csv. As of yet, we can't make this update in the CPS file, but as @MattHJensen pointed out in issue #311, there are users who would like to see this added to the PUF as a first step.

Before merging, I think we should establish what to do with the nu05 variable in the CPS file. I'm inclined to either a) remove it or b) actually keep it in the PUF so that we will have nu05 and nu06. I'm open to other's suggestions on how to move forward from here.

Also, I found that when I created the new PUF file, the e19200 variable sum increased by 1. This is probably just rounding error, but I think it's still odd that this has happened.

@MattHJensen
Copy link
Contributor

MattHJensen commented Nov 4, 2019

Thanks, @andersonfrailey!

Before merging, I think we should establish what to do with the nu05 variable in the CPS file. I'm inclined to either a) remove it or b) actually keep it in the PUF so that we will have nu05 and nu06. I'm open to other's suggestions on how to move forward from here.

I'm inclined to keep both in puf.csv until we add nu06 to cps.csv, at which time we drop nu05 from both files, cps.csv and puf.csv. I don't feel strongly about this though if someone else has an alternative preference

(cc @MaxGhenis).

@MaxGhenis
Copy link
Contributor

I'm not aware of major policies using nu05 so up to you how long to keep it once nu06 is added (replacing would be OK).

@MattHJensen
Copy link
Contributor

@andersonfrailey, are you happy with this approach:

I'm inclined to keep both [nu05 and nu06] in puf.csv until we add nu06 to cps.csv, at which time we drop nu05 from both files, cps.csv and puf.csv.

@andersonfrailey
Copy link
Collaborator Author

@MattHJensen Yep this is good with me. I'll update the PR accordingly.

@andersonfrailey
Copy link
Collaborator Author

Added nu05 back to the PUF in the latest commit

@MattHJensen
Copy link
Contributor

MattHJensen commented Jun 16, 2020

@andersonfrailey, could you update me on the status of this PR when you have an opportunity? I apologize if it was ready to be merged before and requires an update now.

@andersonfrailey
Copy link
Collaborator Author

@MattHJensen, ready to go! There doesn't seem to be any conflicts. We still can't add nu06 to the CPS until PR #314 is merged, but I think that one is ready to go as well.

@andersonfrailey
Copy link
Collaborator Author

Now that #314 is merged, this will be ready to go when I get the merge conflicts resolved

@andersonfrailey
Copy link
Collaborator Author

Fixed up the merge conflicts. Totally removed nu05, tests are passing. This is ready to go
@MattHJensen

@andersonfrailey
Copy link
Collaborator Author

Doesn't look like there are any more comments so merging

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

Successfully merging this pull request may close these issues.

3 participants