pfmc-assessments / PacFIN.Utilities

R code to manipulate data from the PacFIN database for assessments
http://pfmc-assessments.github.io/PacFIN.Utilities
Other
7 stars 2 forks source link

Sex specific comps in writeComps #6

Closed chantelwetzel-noaa closed 5 years ago

chantelwetzel-noaa commented 7 years ago

Need to add if statement at the end of the function based on the value specified for "out" in the function input (Mout, Fout, FthenM). Currently, it writes all options where the FthenM option is the final output that the user gets.

andi-stephens-NOAA commented 7 years ago

That was by design, actually, the idea being that if you later decided you wanted the comps in a different format, you'd have all of them handy.

The "out" argument is for debugging purposes (mine, for debugging code, yours, for previewing data).

--A.

On Wed, Mar 8, 2017 at 3:59 PM, Chantel Wetzel notifications@github.com wrote:

Need to add if statement at the end of the function based on the value specified for "out" in the function input (Mout, Fout, FthenM). Currently, it writes all options where the FthenM option is the final output that the user gets.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nwfsc-assess/PacFIN.Utilities/issues/6, or mute the thread https://github.com/notifications/unsubscribe-auth/AKdMBp0ZCI5MMk_-w-FMoR7c2vCcCpLVks5rj0DdgaJpZM4MXeG8 .

chantelwetzel-noaa commented 7 years ago

This is deceptive to the user because they can specify Mout or Fout but they still receive FthenM. Removing the function input would clarify to the user that they will always receive FthenM.

On Mon, Mar 27, 2017 at 9:41 AM, Andi Stephens notifications@github.com wrote:

That was by design, actually, the idea being that if you later decided you wanted the comps in a different format, you'd have all of them handy.

The "out" argument is for debugging purposes (mine, for debugging code, yours, for previewing data).

--A.

On Wed, Mar 8, 2017 at 3:59 PM, Chantel Wetzel notifications@github.com wrote:

Need to add if statement at the end of the function based on the value specified for "out" in the function input (Mout, Fout, FthenM). Currently, it writes all options where the FthenM option is the final output that the user gets.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nwfsc-assess/PacFIN.Utilities/issues/6, or mute the thread https://github.com/notifications/unsubscribe-auth/AKdMBp0ZCI5MMk_-w- FMoR7c2vCcCpLVks5rj0DdgaJpZM4MXeG8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nwfsc-assess/PacFIN.Utilities/issues/6#issuecomment-289511115, or mute the thread https://github.com/notifications/unsubscribe-auth/AF4tzuRGqs9YpGvBwR5ErnMeeA2w40fyks5rp-ahgaJpZM4MXeG8 .

andi-stephens-NOAA commented 7 years ago

I agree. I think Kelli put that "out" argument in when familiarizing herself with the code.

On Mon, Mar 27, 2017 at 9:53 AM, Chantel Wetzel notifications@github.com wrote:

This is deceptive to the user because they can specify Mout or Fout but they still receive FthenM. Removing the function input would clarify to the user that they will always receive FthenM.

On Mon, Mar 27, 2017 at 9:41 AM, Andi Stephens notifications@github.com wrote:

That was by design, actually, the idea being that if you later decided you wanted the comps in a different format, you'd have all of them handy.

The "out" argument is for debugging purposes (mine, for debugging code, yours, for previewing data).

--A.

On Wed, Mar 8, 2017 at 3:59 PM, Chantel Wetzel <notifications@github.com

wrote:

Need to add if statement at the end of the function based on the value specified for "out" in the function input (Mout, Fout, FthenM). Currently, it writes all options where the FthenM option is the final output that the user gets.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nwfsc-assess/PacFIN.Utilities/issues/6, or mute the thread https://github.com/notifications/unsubscribe-auth/AKdMBp0ZCI5MMk_-w- FMoR7c2vCcCpLVks5rj0DdgaJpZM4MXeG8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nwfsc-assess/PacFIN.Utilities/issues/6#issuecomment- 289511115, or mute the thread https://github.com/notifications/unsubscribe-auth/ AF4tzuRGqs9YpGvBwR5ErnMeeA2w40fyks5rp-ahgaJpZM4MXeG8 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nwfsc-assess/PacFIN.Utilities/issues/6#issuecomment-289514502, or mute the thread https://github.com/notifications/unsubscribe-auth/AKdMBru-Y5s4exudCkBqG2SlCf-_acLtks5rp-l5gaJpZM4MXeG8 .

iantaylor-NOAA commented 7 years ago

The problem discussed in this issue has been addressed in recent commit a7a299d5575f0dcfa341b11b93c359e6ecf6ef01.

As part of that change, the "out" argument has been renamed "returns".

iantaylor-NOAA commented 5 years ago

It turns out that this bug wasn't quite fixed in 2017 after all. Lines 465-466 of the commit referenced above are missing an append = TRUE so the first 3 tables in the file have always been overwritten when the 4th table was written:

  IDstring = paste("\n\n", "Females then males")
  cat(file=fname, IDstring, "\n")

This is fixed in commit 37150c9a3e204711c555c623aca8fe68931e39d3 of the ian_suggestions branch, but should probably be fixed in master if it's not going to mess up anyone's workflow.

kellijohnson-NOAA commented 5 years ago

Fixed in master with af5fbf40fa8b7508bd7826b6250b0d20ebcf2e01 that ensures the first write to the file opens a new file and then the remaining comp types are appended.