Refactor Differential Entropy for Mutual Information Classification

[atavory]

== RELEASE NOTES ==

General Changes

• Move strategy factory code from udf to base class of strategy
• Add population counts/weights to reservoir samples

Together with #13203, this forms the basis for mutual information classification udfs #13163.

[rschlussel]

I looked over it generally, and found that there was way to much going on in this commit to understand easily.

1. I noted a few places, that I felt like could be extracted into separate commits. If there are other bits of refactoring that can stand on their own, you can create separate commits for those too (all commits should be part of this PR, but having the commits more granular makes it way easier to review)
2. It would be helpful to have an overview of what you are doing in this refactoring and for what purpose
3. It might be difficult to evaluate whether the refactoring makes sense without seeing its intended use, so it could be helpful to have a preliminary pr (or commit added to this PR) for the function you're using the refactoring for.

[rschlussel]

add the getTotalPopulationWeight() function + implementations + tests in a separate commit. also, what's it for? it's only used in tests.

[atavory]

It's used in #13163. I could move it there, but, as we discussed, this PR is basically changes that are needed for #13163, so not sure why to move this one in particular.

[rschlussel]

To clarify, I'm not suggesting moving changes to the other PR. I'm asking if you can separate out the different logical changes in this PR into different commits so it's easier to review. You can have multiple commits in a single PR (all the commits on a branch since master will be included in the PR), and github lets you review a PR commit by commit, so breaking this down into more granular changes would be very very helpful.

Examples of independent logical changes for this pr would be: add mehod x to the interface + implementations, the changes in choosing a strategy (and can you provide an overview of what's going on there/ overall goal. i don't quite understand it), random variable name changes to improve clarity.

The easiest way to break apart your commit in git would probably be git reset HEAD^ and then git add -p to select the changes you want for each commit.

[atavory]

The easiest way to break apart your commit in git would probably be git reset HEAD^ and then git add -p to select the changes you want for each commit.

Worked great, thanks!

[rschlussel]

The general approach seems good, but it's still not separated into commits based on the independent logical changes.. I know it can be a bit annoying, but I think it's essential here to break the commits down well because this change touches a lot of code, and it's hard to follow otherwise. Also, each commit should compile on its own, and as currently written, Rename + expand differential entropy calculations utility class would not compile on its own because you haven't changed the places that call that method. Can you break down Refactor factory code into differential_entropy_state_strategy base into multiple commits as follows.

1. Extract calculateEntropy() from FixedHistogramJacknifeStrategy - this commit should be combined with Rename + expand differential entropy calculations utility class, so that in a single commit you have
   a)rename the calculateEntropyFromSamples -> calculateEntropyFromHistogramAggregates
   b) delete code from FixedHistogramJacknifeStrategy
   c) add it to EntropyCalculations
   d) update all the references of both of those methods

2. Rename differential entropy related variables for clarity - this commit would have miscellaneous variable renames, such as samples -> reservoir and bucketCount -> size. Putting this into its own commit, just makes it easier to see the important changed lines in the commits that actually changes. (you can combine this with the documentation change commit if you want, or not whatever you prefer)

3. Add getTotalPopulationWeight() to DifferentialEntropyStateStrategy -- This would include adding the method, all it's implementations, and also tests

4. Add cloneEmpty() to DifferentialEntropyStateStrategy -- including method, it's implementations, and tests

5. Move logic for handling different strategy implementations to DifferentialEntropyStateStrategy - this is the main change that moves the logic for handling which strategy to use and their particularities out of DifferentialEntropyAggregates and DifferentialEntropyStateSerializer into DifferentialEntropyStateStrategy.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Ami Tavory (25f7c55, 26853fc, 2de474e9bbbf5cb1ceb68965eb4b949b4b0bbe19, 03413b14783938f73d18087c18dcb8f0c9d5ce3a, 6770e3f013907e8f05d977bbd4128a7dbfd4e972, 4b0ead1454bf7876e7f6e423e10d680f8c826825)

[rschlussel]

nit: static import

[rschlussel]

just need to move your recent fixes into the logic moving commit to make sure that that one is correct on its own. Otherwise looks good! Thanks!

[caithagoras]

@atavory @rschlussel Please enclose the release notes in a pair of triple backticks(```) in the future. Thanks!

[rschlussel]

Thanks @caithagoras. These change aren't user facing, so actually no release notes needed. Sorry for the trouble!