Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new approach for weight composition by implementing a rebinning function that redistributes frequency data from one set of bin boundaries to another. The change addresses issue #751 by adding a new rebin function and modifying the size frequency processing logic to use rebinning instead of the previous overlap-based distribution method.
Changes:
- Added a new
rebinfunction inSS_miscfxn.tplthat proportionally redistributes frequency data based on bin overlap - Modified size frequency processing in
SS_expval.tplto use the new rebinning approach for length-based bins - Added extensive debug logging throughout the size frequency processing code
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| SS_miscfxn.tpl | Implements the new rebin function for proportional frequency redistribution across bin boundaries |
| SS_expval.tpl | Integrates rebinning for length-based size frequencies and adds debug logging to track bin assignment logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @return Vector of rebinned frequency data (size M). | ||
| */ | ||
|
|
||
| dvector dest_counts(dest_edges.size() - 1, 0.0); |
There was a problem hiding this comment.
Incorrect constructor syntax for dvector initialization. ADMB dvectors use index ranges, not size-based construction. Should be dvector dest_counts(1, dest_edges.size() - 1); dest_counts.initialize();
| dvector dest_counts(dest_edges.size() - 1, 0.0); | |
| dvector dest_counts(1, dest_edges.size() - 1); | |
| dest_counts.initialize(); |
| dvector dest_counts(dest_edges.size() - 1, 0.0); | ||
| for (int i = 0; i < dest_counts.size(); ++i) { | ||
| double d_low = dest_edges[i]; | ||
| double d_high = dest_edges[i + 1]; | ||
|
|
||
| for (int j = 0; j < src_counts.size(); ++j) { | ||
| double s_low = src_edges[j]; | ||
| double s_high = src_edges[j + 1]; | ||
|
|
||
| // Calculate the overlap between [d_low, d_high] and [s_low, s_high] | ||
| double overlap_low = max(d_low, s_low); | ||
| double overlap_high = min(d_high, s_high); | ||
|
|
||
| if (overlap_low < overlap_high) { | ||
| double overlap_width = overlap_high - overlap_low; | ||
| double src_bin_width = s_high - s_low; | ||
|
|
||
| // Distribute source count proportionally to the overlap area | ||
| dest_counts[i] += src_counts[j] * (overlap_width / src_bin_width); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ADMB dvector indexing is 1-based by default, not 0-based. This loop should iterate from the vector's indexmin() to indexmax(), or adjust the initialization to use 0-based indexing with dvector dest_counts(0, dest_edges.size() - 2)
| dvector dest_counts(dest_edges.size() - 1, 0.0); | |
| for (int i = 0; i < dest_counts.size(); ++i) { | |
| double d_low = dest_edges[i]; | |
| double d_high = dest_edges[i + 1]; | |
| for (int j = 0; j < src_counts.size(); ++j) { | |
| double s_low = src_edges[j]; | |
| double s_high = src_edges[j + 1]; | |
| // Calculate the overlap between [d_low, d_high] and [s_low, s_high] | |
| double overlap_low = max(d_low, s_low); | |
| double overlap_high = min(d_high, s_high); | |
| if (overlap_low < overlap_high) { | |
| double overlap_width = overlap_high - overlap_low; | |
| double src_bin_width = s_high - s_low; | |
| // Distribute source count proportionally to the overlap area | |
| dest_counts[i] += src_counts[j] * (overlap_width / src_bin_width); | |
| } | |
| } | |
| } | |
| // dest_counts has one fewer element than dest_edges and follows the same index base. | |
| dvector dest_counts(dest_edges.indexmin(), dest_edges.indexmax() - 1); | |
| dest_counts.initialize(); | |
| for (int i = dest_counts.indexmin(); i <= dest_counts.indexmax(); ++i) | |
| { | |
| double d_low = dest_edges(i); | |
| double d_high = dest_edges(i + 1); | |
| for (int j = src_counts.indexmin(); j <= src_counts.indexmax(); ++j) | |
| { | |
| double s_low = src_edges(j); | |
| double s_high = src_edges(j + 1); | |
| // Calculate the overlap between [d_low, d_high] and [s_low, s_high] | |
| double overlap_low = max(d_low, s_low); | |
| double overlap_high = min(d_high, s_high); | |
| if (overlap_low < overlap_high) | |
| { | |
| double overlap_width = overlap_high - overlap_low; | |
| double src_bin_width = s_high - s_low; | |
| // Distribute source count proportionally to the overlap area | |
| dest_counts(i) += src_counts(j) * (overlap_width / src_bin_width); | |
| } | |
| } | |
| } |
| dvector dest_counts(dest_edges.size() - 1, 0.0); | ||
| for (int i = 0; i < dest_counts.size(); ++i) { | ||
| double d_low = dest_edges[i]; | ||
| double d_high = dest_edges[i + 1]; | ||
|
|
||
| for (int j = 0; j < src_counts.size(); ++j) { | ||
| double s_low = src_edges[j]; | ||
| double s_high = src_edges[j + 1]; | ||
|
|
||
| // Calculate the overlap between [d_low, d_high] and [s_low, s_high] | ||
| double overlap_low = max(d_low, s_low); | ||
| double overlap_high = min(d_high, s_high); | ||
|
|
||
| if (overlap_low < overlap_high) { | ||
| double overlap_width = overlap_high - overlap_low; | ||
| double src_bin_width = s_high - s_low; | ||
|
|
||
| // Distribute source count proportionally to the overlap area | ||
| dest_counts[i] += src_counts[j] * (overlap_width / src_bin_width); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same indexing issue as outer loop. ADMB vectors use 1-based indexing by default. Should iterate from src_counts.indexmin() to src_counts.indexmax()
| dvector dest_counts(dest_edges.size() - 1, 0.0); | |
| for (int i = 0; i < dest_counts.size(); ++i) { | |
| double d_low = dest_edges[i]; | |
| double d_high = dest_edges[i + 1]; | |
| for (int j = 0; j < src_counts.size(); ++j) { | |
| double s_low = src_edges[j]; | |
| double s_high = src_edges[j + 1]; | |
| // Calculate the overlap between [d_low, d_high] and [s_low, s_high] | |
| double overlap_low = max(d_low, s_low); | |
| double overlap_high = min(d_high, s_high); | |
| if (overlap_low < overlap_high) { | |
| double overlap_width = overlap_high - overlap_low; | |
| double src_bin_width = s_high - s_low; | |
| // Distribute source count proportionally to the overlap area | |
| dest_counts[i] += src_counts[j] * (overlap_width / src_bin_width); | |
| } | |
| } | |
| } | |
| // ADMB vectors are 1-based by default; derive index ranges from the edge vectors. | |
| int dest_min = dest_edges.indexmin(); | |
| int dest_max = dest_edges.indexmax() - 1; // last bin uses dest_edges(dest_max+1) | |
| dvector dest_counts(dest_min, dest_max); | |
| dest_counts.initialize(); | |
| for (int i = dest_min; i <= dest_max; ++i) | |
| { | |
| double d_low = dest_edges(i); | |
| double d_high = dest_edges(i + 1); | |
| int src_min = src_counts.indexmin(); | |
| int src_max = src_counts.indexmax(); | |
| for (int j = src_min; j <= src_max; ++j) | |
| { | |
| double s_low = src_edges(j); | |
| double s_high = src_edges(j + 1); | |
| // Calculate the overlap between [d_low, d_high] and [s_low, s_high] | |
| double overlap_low = max(d_low, s_low); | |
| double overlap_high = min(d_high, s_high); | |
| if (overlap_low < overlap_high) | |
| { | |
| double overlap_width = overlap_high - overlap_low; | |
| double src_bin_width = s_high - s_low; | |
| // Distribute source count proportionally to the overlap area | |
| dest_counts(i) += src_counts(j) * (overlap_width / src_bin_width); | |
| } | |
| } | |
| } |
| dvector freq_in(1, z2-z1); // fill the input | ||
| freq_in = 1.; | ||
| echoinput << " z1: " << z1 << " z2: " << z2 << endl; | ||
| echoinput << " freq_in: " << freq_in << endl; | ||
| dvector bins_in(1, z2-z1); | ||
| bins_in = len_bins2(z1, z2); // input bins shifted |
There was a problem hiding this comment.
Vector size mismatch. A vector from index 1 to z2-z1 has size z2-z1, but should have size z2-z1+1 to match the number of bins from z1 to z2 inclusive. Should be dvector freq_in(1, z2-z1+1)
| dvector freq_in(1, z2-z1); // fill the input | |
| freq_in = 1.; | |
| echoinput << " z1: " << z1 << " z2: " << z2 << endl; | |
| echoinput << " freq_in: " << freq_in << endl; | |
| dvector bins_in(1, z2-z1); | |
| bins_in = len_bins2(z1, z2); // input bins shifted | |
| dvector freq_in(1, z2-z1+1); // fill the input (one element per bin edge from z1 to z2 inclusive) | |
| freq_in = 1.; | |
| echoinput << " z1: " << z1 << " z2: " << z2 << endl; | |
| echoinput << " freq_in: " << freq_in << endl; | |
| dvector bins_in(1, z2-z1+1); | |
| bins_in = len_bins2(z1, z2); // input bins shifted (edges from z1 to z2 inclusive) |
| freq_in = 1.; | ||
| echoinput << " z1: " << z1 << " z2: " << z2 << endl; | ||
| echoinput << " freq_in: " << freq_in << endl; | ||
| dvector bins_in(1, z2-z1); |
There was a problem hiding this comment.
Vector size and assignment mismatch. bins_in needs z2-z1+2 elements to store bin edges (boundaries) for z2-z1+1 bins. The vector should be sized dvector bins_in(1, z2-z1+2) to accommodate N+1 edges for N bins
| dvector bins_in(1, z2-z1); | |
| dvector bins_in(1, z2-z1+2); |
| dvector bins_in(1, z2-z1); | ||
| bins_in = len_bins2(z1, z2); // input bins shifted | ||
| echoinput << " bins_in: " << bins_in << endl; | ||
| // dvector bins_out(1, z2-z1) = SzFreq_bins(SzFreqMethod |
There was a problem hiding this comment.
Incomplete commented-out code should be removed. This appears to be leftover development code that creates confusion.
| // dvector bins_out(1, z2-z1) = SzFreq_bins(SzFreqMethod |
|
@Rick-Methot-NOAA I had copilot review your PR, idk if some of the suggestions it is making are correct. Some seem useful, but others might not be. Please feel free to press the "Resolve conversation" button for those that aren't useful. |
Concisely describe what has been changed/addressed in the pull request.
What tests have been done?
in progress
Where are the relevant files?
What tests/review still need to be done?
Is there an input change for users to Stock Synthesis?
Additional information (optional).