develop SS_readctl_3.30 and SS_writectl_3.30

[iantaylor-NOAA]

Developing read/write functions for control files seemed to be a very difficult task but @yukio-takeuchi managed to get it done. At this point, those functions only work for SS version 3.24. Developing functions for 3.30 isn't a top priority until users of 3.24 versions want to upgrade. This issue is to note that the 3.30 versions of these functions haven't yet been developed and to track progress if/when we choose to do so.

[kellijohnson-NOAA]

As written SS_readctl_3.24 and SS_readctl do not match in their argument names. I am uncertain which one to change, but Nfleet is wrongly mapped to Nfish and Nsurveys is being pseudo mapped to Nsurv through partial matching. I suggest that the discrepancies be cleared up such that both functions use names that are used throughout r4ss (i.e., Nfleet and Nsurveys). I am happy to do this, I just wanted to make sure that it makes sense to @taylori and @yukio-takeuchi

[iantaylor-NOAA]

Thanks for catching this @kellijohnson.

Since SS_readctl_3.30 hasn't yet been written (@yukio-takeuchi has volunteered to work on it starting in September), I think the few users of these files (including @iagomosqueira) have probably just called SS_readctl_3.24 directly rather than the wrapper function SS_readctl which is out of date so the change should take place there,

The consistency question is a big enough problem that I will post a separate issue (probably will be #140) about it.

[yukio-takeuchi]

I agree that there is some inconsistency of arguments names in between SS_readctl_3.24 and SS_readctl. I remember that those inconsistencies happened when I enhanced SS_readctl_3.24 to cover more options. Until when SS_readctl_3.30 is available ( I hope, I can work on it late this year), I also suggest to call SS_readctl_3.24 directly.

…

2017/04/07 6:45、Ian Taylor ***@***.***>のメール: Thanks for catching this @kellijohnson <https://github.com/kellijohnson>. Since SS_readctl_3.30 hasn't yet been written ***@***.*** <https://github.com/yukio-takeuchi> has volunteered to work on it starting in September), I think the few users of these files (including @iagomosqueira <https://github.com/iagomosqueira>) have probably just called SS_readctl_3.24 directly rather than the wrapper function SS_readctl which is out of date so the change should take place there, The consistency question is a big enough problem that I will post a separate issue (probably will be #140) about it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOy1sX6zl4w8GbeRFb_QqLco_ZTtMr_ks5rtUDrgaJpZM4MwET8>.

[nklaer]

I have SS_readctl_3.30 working for the few example files that I have available, based on Yukio's 3.24 code and called via SS_readctl. I'll talk to Ian Taylor about steps for getting it into R4SS.

[iantaylor-NOAA]

@nklaer, thanks for the good news.
The formal git approach would be to commit the changes in a fork of the repository and then issue a pull request but I'm also perfectly happy to integrate them into the package myself if you just email me the new/modified files.

[iantaylor-NOAA]

Functions from @nklaer have been added in commit adeba65 to the development branch, available via

devtools::install_github('r4ss/r4ss', ref='development')

The functions SS_readctl and SS_readdat now support a much wider array of model versions and configurations, including 3.30.

Please feel free to try these out and provide feedback.

SS_writectl_3.30 has not yet been written.

[k-doering-NOAA]

@ChristineStawitz-NOAA brought up the point that some additional input checks for SS_readctl() may be useful. For example, it should be made clearer that default values for Nfleet and Nsurveys are used(set to 2) if not specified by the user (either by specifying a data list to read from, or specifying the values as function input). Perhaps checks on user input will be helpful and/or the documentation can be updated to make it clearer to users about how to use SS_readctl(). Or maybe it makes sense to remove default value for some of the inputs that do not have a common default value across the majority of models that SS users will try to read in.

[k-doering-NOAA]

Just another thought that @kellijohnson-NOAA has mentioned and I also think may be a good idea. Currently, SS_readctl_3.30() only creates structures in the list if they are necessary to have for the file being read. For example, if natural mortality type = 1, a list component for the number of breakpoints (called N_natM) is created; but if type = 0, there is no component created. There may be some benefits to having this component always included in the list, but have value NULL when not used instead of not existing at all. On the other hand, this type of list may prove difficult to create because the control file structure changes a lot based on the options being used.

Anyone have thoughts on this subject?

[iagomosqueira]

Having list element as NULL might be unnecessary. If you try to access a non-existent list element by name it already returns NULL. The only extra information I can see this giving are the names of possible elements, through names(list).

[k-doering-NOAA]

@iagomosqueira, sorry I forgot to reply, but that is a good point. I agree that seeing the names of all possible elements may be helpful, but perhaps it doesn't add that much value. I had forgotten that trying to access a non-existent list element turns up NULL anyway.

[k-doering-NOAA]

Just found in SS_readctl_3.30 that columns for Prior SD and Prior Type were labelled in the wrong order for MG_parms dataframe (should be Prior SD, then PR Type). The values are in the correct order, though, I think.
https://github.com/r4ss/r4ss/blob/master/R/SS_readctl_3.30.R#L630-L635
I plan on fixing this in the next few days and then submitting a pull request to the development branch.

[k-doering-NOAA]

As usual, when working on an issue, other small fixes become apparent. While fixing the switched positions of the Prior SD and Prior Type columns assumed in SS_readctl_3.30, I realized that the devs phase column (in SS input files, labeled dev_PH) within dataframes for long parameter lines is labeled dev_stddev when created by SS_readctl_3.30 . I think this is a holdover from SS 3.24 where there was a column for devs standard deviation.

Ideally, I think the name of the column should be changed to accurately reflect what it is, but I'm trying to figure out how to do this keeping backcompatibility in mind (see #306 ). I think there are several possibilities, depending on how much backcompatibility is desired (although both these solutions assume backcompatibility is only needed short term).

Some ideas:

1. Easiest: For the time being, generate a warning everytime SS_readctl_3.30 is called informing users that the column name dev_stddev will be changed in the future to dev_PH. (next release??). Then, once the change is made, change the warning to inform users that the column name has changed. This doesn't create backcompatibility or create a transition period where users can use either the new or old column name, but it may help users troubleshoot issues.

2. For the time being, create a new column in these dataframes called dev_PH which contains the same values as the existing dev_stddev column. Also issue a warning everytime the SS_readactl_3.30 function is called that the column name dev_stddev will be deprectated at some point in the future (next release??) The tricky part that I'm not sure how to do is how to tell SS_writectl_3.30 which of these columns to read when writing out. Ideally, if a user manipulates either column, we would want to read out that value, but I can't think of a way for the program to know this without user input (although perhaps others have a solution?). This would create backcompatibility for a short while and be a more gradual change for users than the first point. Another downside of this approach is having an extra column in these dataframes may be confusing for users.

@iantaylor-NOAA or anyone else, thoughts? While a fairly trivial change, I think this is worth discussing because I imagine it will be applicable to other changes within r4ss in the future.

[iantaylor-NOAA]

I suspect that those who contributed earlier versions of SS_readctl_3.30 never used it to access the dev_PH column and so didn't catch the incorrect label (indeed surely a holdover from previous column order/names in 3.24).

I think relatively few people use of this function and since the status-quo method is clearly wrong, what I would do is simply fix it to have the correct label and trust that anyone impacted by the change (which might be truly nobody) will be able to quickly modify their code to adapt to the difference.

My view of backward compatibility in the context of these functions is satisfied by maintaining compatibility with both 3.24 and 3.30 models, but I don't see a need to maintain consistency with earlier versions of r4ss that weren't correctly.

[k-doering-NOAA]

Thanks, Ian - I will go ahead and change the column name and hope others (if any) will adapt. I suspect you are right that it will likely affect very few people, but I guess more care would be needed with functions that are likely used by others.

[k-doering-NOAA]

I just wanted to comment that @nathanvaughan-NOAA and I are working on adding more options to SS_readctl_3.30 and SS_writectl_3.30. The goal is that these could successfully read and write any valid SS 3.30 model, (and we hope that it is not too ambitious!). If anyone else is also working on this, please let us know; perhaps we can find a way to collaborate.

[k-doering-NOAA]

I am closing this issue because pull request #352 adds capabilities to read most 3.30 control files. Note there are likely some bugs because ctl file configuration varies greatly among models and there are many configurations that have not been tested. There may also be some features that do not yet work (search for stop() or TODO within the SS_writectl_3.30 and SS_readctl_3.30 scripts to see some acknowledged limitations). If anyone feels that this issue should be left open given these caveats, feel free to reopen.

[iantaylor-NOAA]

Thanks @k-doering-NOAA and @nathanvaughan-NOAA for all these improvements.
I just updated the development branch to include @nathanvaughan-NOAA as an additional co-author of the package and incremented the package version to 1.36.2.

After today I'll be offline for the rest of the year, but can work on getting development ready for merging into master in the new year.