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

Deprecate ALK tolerance #342

Closed
k-doering-NOAA opened this issue Jul 6, 2022 · 7 comments · Fixed by #380
Closed

Deprecate ALK tolerance #342

k-doering-NOAA opened this issue Jul 6, 2022 · 7 comments · Fixed by #380
Assignees
Labels
change log use for issues that should appear in change log misc. input resolved issue resolved, look for "needs test" label run control warnings
Milestone

Comments

@k-doering-NOAA
Copy link
Contributor

Discussion on nmfs-fish-tools/SSMSE#129 suggests that the ALK tolerance setting in the starter file should be deprecated. Perhaps we could add a warning first to suggest users set it to 0, then in a future release require users to reset it to 0?

@Rick-Methot-NOAA , feel free to comment on this issue if I didn't capture it how you were imagining

@Rick-Methot-NOAA
Copy link
Collaborator

let's retain the FUNCTION that does it, but remove it from conditional statements that invoke it and mark it as deprecated in the starter.ss file

@nschindler-noaa
Copy link
Contributor

This section of code in SS_ALK.tpl is the only place calc_ALK_range is used:
int ALK_phase = 0; if (Grow_logN == 0) { int ALK_finder = (ALK_idx - 1) * gmorph + g; if ((do_once == 1 || (current_phase() > ALK_phase)) && !last_phase()) { ALK_phase = current_phase(); ALK_range_use = calc_ALK_range(len_bins, use_Ave_Size_W, use_SD_Size, ALK_tolerance); // later need to offset according to g ALK_range_g_lo(ALK_finder) = column(ALK_range_use, 1); ALK_range_g_hi(ALK_finder) = column(ALK_range_use, 2); } ALK(ALK_idx, g) = calc_ALK(len_bins, ALK_range_g_lo(ALK_finder), ALK_range_g_hi(ALK_finder), use_Ave_Size_W, use_SD_Size); } else { ALK(ALK_idx, g) = calc_ALK_log(log_len_bins, use_Ave_Size_W, use_SD_Size); }

It can be replaced by just using the "else" clause (the last line). ALK_tolerance is then ignored. We could add a Note when ALK_tolerance is read that says something like "ALK_tolerance is not currently being used, but must remain in the starter.ss file". Or we could use the word "deprecated".

@nschindler-noaa
Copy link
Contributor

Wow, this really messed up the code, didn't it? Here it is without formatting:
int ALK_phase = 0;
if (Grow_logN == 0)
{
int ALK_finder = (ALK_idx - 1) * gmorph + g;
if ((do_once == 1 || (current_phase() > ALK_phase)) && !last_phase())
{
ALK_phase = current_phase();
ALK_range_use = calc_ALK_range(len_bins, use_Ave_Size_W, use_SD_Size, ALK_tolerance); // later need to offset according to g
ALK_range_g_lo(ALK_finder) = column(ALK_range_use, 1);
ALK_range_g_hi(ALK_finder) = column(ALK_range_use, 2);
}
ALK(ALK_idx, g) = calc_ALK(len_bins, ALK_range_g_lo(ALK_finder), ALK_range_g_hi(ALK_finder), use_Ave_Size_W, use_SD_Size);
}
else
{
ALK(ALK_idx, g) = calc_ALK_log(log_len_bins, use_Ave_Size_W, use_SD_Size);
}

@Rick-Methot-NOAA
Copy link
Collaborator

I find a few more instances of ALK_range. See attached file
ALK_range.txt

@Rick-Methot-NOAA Rick-Methot-NOAA added in progress This is being worked on in a branch and removed new request initial entry of a new request labels Nov 16, 2022
@nschindler-noaa
Copy link
Contributor

Thanks. I have been commenting out ALK_range stuff.
One item needs resolution: calc_ALK requires an imatrix for low and high lengths (?). I can force it internally to 1 and nlength (which I did and it works), or I can change the calling parameters. It is only used in Make_Agelength_Key.

@Rick-Methot-NOAA
Copy link
Collaborator

OK to force internally to 1, nlength

@nschindler-noaa
Copy link
Contributor

Ok. Creating pull request.

@nschindler-noaa nschindler-noaa linked a pull request Nov 17, 2022 that will close this issue
6 tasks
@e-perl-NOAA e-perl-NOAA added the change log use for issues that should appear in change log label Dec 15, 2022
@Rick-Methot-NOAA Rick-Methot-NOAA added resolved issue resolved, look for "needs test" label and removed in progress This is being worked on in a branch labels Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change log use for issues that should appear in change log misc. input resolved issue resolved, look for "needs test" label run control warnings
Projects
Status: No status
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants