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

default value of comp_bins throws error in writeComps #153

Open
brianlangseth-NOAA opened this issue Feb 20, 2025 · 3 comments
Open

default value of comp_bins throws error in writeComps #153

brianlangseth-NOAA opened this issue Feb 20, 2025 · 3 comments
Assignees

Comments

@brianlangseth-NOAA
Copy link
Contributor

Describe the bug
When running writeComps, the default value for comp_bins (which is NULL) results in an error, which happens in lines 215-218 of the code with the error

Error in UseMethod("full_seq") :
no applicable method for 'full_seq' applied to an object of class "NULL"

To Reproduce
Steps to reproduce the behavior:

Run comp calculation up to getComps, then

Lcomps = pacfintools::getComps(Pdata_exp, Comps = "LEN")

pacfintools::writeComps(inComps = Lcomps, 
           fname = file.path(here("data", "CAquillback_PacFIN_LengthComps.csv")), 
           lbins = length_bins,
           partition = 0, 
           digits = 4)

Expected behavior
I expect the defaults to run. When I use comp_bins = seq(12,66,2), the code works.

Additional context
I suggest that when no comp_bins are provided then have the code automatically set it to some range based on the data. Alternatively remove the default and just prompt the user that they need to enter something.

Altogether the issue is resolveable, just needs to be communicated better.

@iantaylor-NOAA
Copy link
Contributor

@brianlangseth-NOAA, thanks for flagging this. I agree that the error from the defaults is bad and some change is needed.
I don't like the idea of providing the user with an automatically generated set of bins because in general the user is going to want a specific set of bins based on all the data sources in their model, not just this one source. I think a quick and easy fix would be to remove the NULL default so the function just warns you that you need to provide a value for comp_bins rather than a confusing error.

@chantelwetzel-noaa
Copy link
Contributor

@iantaylor-NOAA I made the same comment that we probably shouldn't be automatically generating bins when not provided by the user when talking with @brianlangseth-NOAA about this issue. I am working on a code addition in getComps() and can remove the NULL in the comp_bins argument when I push my changes.

@iantaylor-NOAA
Copy link
Contributor

@chantelwetzel-noaa, thanks for stepping up!

I see that there's some code related to comp_bins = NULL in these lines:

pacfintools/R/writeComps.R

Lines 230 to 235 in bad3513

if (is.null(comp_bins)) {
if (verbose) {
cli::cli_alert_info("No composition bins provided, using data as-is.")
}
comp_bins <- sort(unique(inComps[["comp_type"]]))
}
but I'm guessing that nobody has been using that option since nobody until @brianlangseth-NOAA has reported this error. Therefore we could just remove that part and update the roxygen description for comp_bins.

FYI, the error if there's no default should look like

Error in pacfintools::writeComps() :
  argument "comp_bins" is missing, with no default

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

No branches or pull requests

3 participants