[leewujung]

This PR aims to address #535 :

• Simplified binify() by absorbing binned_distribution()
• Compute internal edges from consecutive bin midpoints, which corrects the current problematic uneven bin handling
• Throw out a warnings when input values fall outside all of the binning intervals
• Consolidated binning tests and add tests to actually check the output values, especially for the downstream length_binned_weights function (the calculation in this function can be simplified in a separate PR)

Questions needing discussion:

• rename binify() to add_binning_intervals() -- it better describes what the function does, which is to add a column with the binning interval to the input dataframe
• rename the input argument bin_column to column_to_bin

@rebeccathomas-NOAA : the questions above would require changing the workflow files. These are simple changes that can be easily done, but if we make the changes, all of us should make that changes at the same time, so that nothing breaks.

---

NOTE: I found out the tests for the functions I have reviewed so far did not test for the actual output values. This is problematic because then there is no guarantee on whether the functions are producing the right outputs. As I go through all the implementations in detail I'll make sure to add these really important tests.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[leewujung]

From the meeting on Oct 13 we decided that we'll just use bin edges so that the bins are explicitly defined from the outset, instead of inferred based on the presumed bin centers. This brings the behavior of the binify function pretty much just a wrapper for pd.cut.

After reviewing the use cases of binify, I think it is cleaner to just use pd.cut and explicitly loop through the 2 dataframes (length, specimen) when needed (previously the binify function accepts both dataframe or dict of dataframe as inputs). This PR now contains these changes.

The changes require updates of the workflow files. Here all the workflow files are updated, but I just wanted to make a note that these files are not what's being actively used in the FEAT team. We'll remove them once the function/workflow code becomes more stable.

@rebeccathomas-NOAA : I will not merge until we meet on Thursday, because if we merge this your workflow will break. Let's figure out a good way to merge code changes and then update your workflow to run with the new code before we merge anything.

[rebeccathomas-NOAA]

Thanks for working on this! Some questions:

1. I haven't used ReviewNB before. Is the code in there something you'd like me to look it, or was that more for Domenic's benefit. I guess one needs to make an account on ReviewNB to use it?
2. I am fine with waiting until Thursday to update the workflow. It would be nice to have it spelled out very clearly for me which workflow things need to be changed as well as settings (e.g. how to notate the age and length bin calls). Maybe that's what ReviewNB is for?
   Thanks,
   RT

[leewujung]

So ReviewNB in the old times provides better visibility of notebook changes than the GitHub native views (though I think GitHub is pretty good now, so maybe we can remove that at some point). The notebooks that got changed are just the documentation.

You could check one of the hake_YEAR.py for the actual places the code needs to change. It’s pretty minimal- just the call to do the length and age binning, and the length bin and age bin specifications. And yeah it’ll be good to do it together when we meet.

[leewujung]

I'm self-merging this after the Oct 22 meeting where we agreed that merging this and for @rebeccathomas-NOAA and @dbashford-NOAA to update the affected scripts separately is the way to go.