Natural gradient gaussian + shuffle in CV with hyperopt + skip problematic HP combindations

[ujohn33]

Here I implemented the analytical computation of FIM for Gaussian DGBM natural gradient learning, FIM calculation for other distributions can be implemented in the same manner based on the analytical closed-form formulation of FIM (see https://arxiv.org/abs/2304.14885)

The hyperopt implementation was also updated with minor improvements, its now possible to set shuffle=False (important for time-series), and skip problematic hyperparameter combinations (like when min_data_in_leaf is too small) and continue optimization instead of crashing

[StatMixedML]

@claude Review the PR in detail

[claude]

Claude finished @StatMixedML's task —— View job

---

PR Review - Branch Synchronization Issue

I've attempted to review this PR but encountered a critical issue: the natural_gradient branch does not contain the changes described in the PR.

Expected Changes (from PR metadata):

• ✗ New file: docs/examples/Gaussian_natural.ipynb (+1529 lines)
• ✗ New file: lightgbmlss/distributions/LogitNormal.py (+71 lines)
• ✗ New directory: tests/test_natural_gradient/ (+656 lines)
• ✗ Modified: Gaussian.py (natural gradient FIM implementation)
• ✗ Modified: model.py (hyperopt with shuffle parameter)
• ✗ Modified: distribution_utils.py

Actual Branch State:

The natural_gradient branch only contains commits from March 2024 and earlier, with the latest being "Update loss calculation in metric_fn". None of the files mentioned in the PR description exist in the current branch state.

What Happened:

This appears to be a branch synchronization issue. The changes likely exist in a local repository but haven't been pushed to GitHub's origin/natural_gradient branch, or there was a force-push that reverted the branch to an earlier state.

Next Steps:

@StatMixedML To resolve this, please:

1. Verify local changes exist:

   git log natural_gradient --oneline | head -20

   You should see commits related to natural gradient implementation.

2. Push the changes:

   git push origin natural_gradient --force-with-lease

   (The --force-with-lease is safer than --force as it won't overwrite others' changes)

3. Alternatively, if changes are on a different local branch:

   git push origin <your-local-branch>:natural_gradient --force-with-lease

Once the branch is properly synchronized, I'll be able to review:

• The analytical FIM computation for Gaussian natural gradient learning
• The hyperopt improvements (shuffle parameter, error handling)
• The new LogitNormal distribution implementation
• Test coverage for the natural gradient functionality

---

[ujohn33]

@claude Review the PR in detail

I think the problem with claude review is that there is a natural_gradient branch both in my fork and the original repo, so it reviews the latter one

[StatMixedML]

@claude Review the PR in detail

I think the problem with claude review is that there is a natural_gradient branch both in my fork and the original repo, so it reviews the latter one

@ujohn33 Can you try and fix it, thanks.

[claude]

Claude finished @StatMixedML's task —— View job

---

PR Review - Branch Access Issue

I've identified the problem and understand why the previous review failed. However, I'm encountering a permissions limitation that prevents me from completing the review.

The Problem

This PR is from ujohn33/LightGBMLSS:natural_gradient → StatMixedML/LightGBMLSS:master, but the GitHub Action is checking out StatMixedML/LightGBMLSS:natural_gradient instead of the fork's branch. This means I'm looking at the wrong code - the base repository's natural_gradient branch (which has old commits from March 2024) rather than ujohn33's fork's natural_gradient branch (which contains the new natural gradient implementation).

What I Tried

I attempted to:

1. ✅ Identify the issue (confirmed it's a fork branch vs base branch problem)
2. ❌ Add ujohn33's fork as a git remote (requires approval/permissions I don't have)
3. ❌ Fetch the correct branch from the fork (blocked by the above)

Current Branch State

The branch I have access to (StatMixedML/LightGBMLSS:natural_gradient) only contains:

• Latest commit: "Update loss calculation in metric_fn" (3ba04b7)
• No natural gradient FIM implementation
• No LogitNormal.py file
• No Gaussian_natural.ipynb notebook
• No test_natural_gradient/ directory

Solutions

Option 1: GitHub Action Configuration (Recommended)
The GitHub Action workflow needs to be configured to check out the PR head ref instead of the base repository's branch. This is typically done with:

- uses: actions/checkout@v4
  with:
    ref: ${{ github.event.pull_request.head.sha }}
    repository: ${{ github.event.pull_request.head.repo.full_name }}

However, I cannot modify workflow files due to permission restrictions.

Option 2: Merge to Base Branch First
@ujohn33 could push the natural_gradient changes to the base repository (StatMixedML/LightGBMLSS:natural_gradient), then the PR would work correctly. This would involve:

# From ujohn33's local repository
git push https://github.com/StatMixedML/LightGBMLSS.git natural_gradient

(This requires push access to StatMixedML/LightGBMLSS)

Option 3: Manual Review
@StatMixedML or @ujohn33 could:

1. Share the fork URL explicitly in a comment
2. Grant me additional permissions to fetch from external repositories
3. Or I could review specific files if you paste them in comments (not ideal for 2500+ lines of changes)

What Can Be Reviewed (if we fix the access)

Once I have access to the correct branch, I can review:

• ✅ Analytical FIM computation for Gaussian natural gradient (Gaussian.py +36/-2)
• ✅ Hyperopt improvements with shuffle parameter (model.py +49/-56)
• ✅ Distribution utility updates (distribution_utils.py +47/-29)
• ✅ New LogitNormal distribution implementation (LogitNormal.py +71 lines)
• ✅ Test coverage (test_natural_gradient_gaussian.py +481 lines)
• ✅ Documentation and examples (Gaussian_natural.ipynb +1529 lines)

---

Next Steps: Please choose one of the options above so I can proceed with the detailed review. Option 2 (pushing to the base repository's natural_gradient branch) would be the quickest fix if @ujohn33 has the necessary permissions.

---

[nucflash]

Hi @ujohn33 , this is a cool PR! It feels that it addresses more than one thing which makes it hard to review. Would it be possible to split it into three PRs?

1. Implement LogitNormal + unit tests
2. Implement FIM for all dists (by adjusting the root class of all dists) + unit tests
3. Suggest fix for shuffle + the low/high bound issue + unit tests

Wrt unit tests, it would be good if your unit tests fit into the existing ones by Alexander, I think he has done a very good job at organizing them. If you are using an LLM: after the LLM proposes a new unit test file, point it to the relevant actual unit test file and ask it to adjust its unit test to fit into the file (and, if possible match the already existing unit tests—e.g., by adding assertions, not by changing their logic).

Wrt (3) — I believe that no shuffling or stratification is actually done despite what is said in the docstring:

LightGBMLSS/lightgbmlss/model.py

Lines 223 to 226 in 0f9db92

	stratified : bool, optional (default=True)
	Whether to perform stratified sampling.
	shuffle : bool, optional (default=True)
	Whether to shuffle before splitting data.

LightGBMLSS/lightgbmlss/model.py

Lines 275 to 276 in 0f9db92

	stratified=False,
	shuffle=False,

I'm not sure if this is intended or not, or what is its significance or repercussion. I'm only pointing it out. Probably it's worth an issue and a treatment of its own (change of the docstring, or of how the variables are set).

I hope this helps. Thanks again for your work, and I'm looking forward to seeing it merged into the package soon!