first commit of rebin - #783
Conversation
|
please do not link copilot until I am ready |
|
get a match to the legacy method using this setup: Beware: lots of temporary echoinput: statements still in the output.
|
|
@Rick-Methot-NOAA while on my own I don't fully understand what each line of code is doing, I did have Gemini explain it to me and I asked it if it had any suggestions for improvement and it suggested two things:
The first suggestion seems logical and harmless enough. The second suggestion, I don't fully understand but is only a change for computational savings. |
|
Thanks Elizabeth. The second suggestion is something I was already thinking about and is what the legacy code was already capable of. I implemented an efficient search so that each source bin is only addressed once or twice for each destination bin. Sequence from some temporary echoinputs looks like this: Next push will be for the streamlined search algorithm. looping in @N-DucharmeBarth-NOAA who prompted this work long ago. |
iantaylor-NOAA
left a comment
There was a problem hiding this comment.
@Rick-Methot-NOAA, thanks for moving this forward.
I didn't try to fully understand the algorithm but ran it with the Simple_with_DM_sizefreq model and the results are identical to 3.30.25.1, so that's good enough for me in terms of the calculations.
Regarding efficiency, I ran the model before and after the latest commit and unfortunately things are running slower (based on sample size of 1 with each version).
- 3.30.25.1 model (run with -nohess): 373 iterations, 35 seconds
- before latest commit: 373 iterations, 49 seconds
- after latest commit: 373 iterations, 1 minutes, 33 seconds
Could the echoinput.sso be slowing things down? The rebin versions have 94,000+ lines vs ~2800 in the original (although that doesn't explain the difference caused by the latest commit). In the final version of this PR, it would be good to reduce the echoinput and remove the legacy rebin calculations entirely (perhaps that's already noted somewhere as the plan).
| SzFreq_means(k, z + SzFreq_Nbins(k)) = SzFreq_means(k, z); | ||
| } | ||
| } | ||
| // SzFreq_bins2(k, SzFreq_Nbins(k)) = 99999.; |
There was a problem hiding this comment.
I think commented-out lines like these should either be removed or have a comment above them noting what they would do if they were added back
| // the logic that created SzFreqTrans needs to be converted into logic that finds the length (in cm) that corresponds to the destination bin boundaries. | ||
| // This allows rebin to go from cm to cm | ||
| // with rebin it seems better to convert the numbers to weight to create an exp_wt_temp, then apply rebin to parse those weights to the new bins, using the bin boundaries that are in cm | ||
| //accumulate body weight into the bins and |
There was a problem hiding this comment.
comment here seems unfinished
|
One more comment. I modified the "Simple_with_DM_sizefreq" model to turn off the movement and the time-varying allocation of recruitment among areas which have different growth patterns: control.ss.txt. This causes the |
|
Thanks for taking a look Ian, but this is still not ready for any real testing. The expected values are still coming from the old method and all my testing has just been unit testing within that code segment. I should have just looped you in with an @, rather that letting it seem like it is ready for review. |
|
@Rick-Methot-NOAA no problem. It's good to start understanding how this is working even if it's not ready for prime time yet. |
Concisely describe what has been changed/addressed in the pull request.
This PR adds a FUNCTION "rebin" in miscfxn.tpl then attempts to use it as a better way to implement generalized sizecomp data
What tests have been done?
Where are the relevant files?
-- - [x] Test files are in the issue. -->
What tests/review still need to be done?
Is there an input change for users to Stock Synthesis?
Additional information (optional).