feat: implement phase unwrapping to fix binning for wrapped eclipses #5
+1,327
−198
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add _detect_wrapped_eclipse and _calculate_unwrap_shift methods to detect and handle eclipses that wrap around the phase 0/1 boundary. Includes test to verify detection of wrapped secondary eclipse.
- Modified __init__ to detect and unwrap wrapped eclipses at initialization - Added _unwrap_phases() method to apply phase shift and re-sort data - Improved _calculate_unwrap_shift() to properly unwrap eclipses without wrapping the other - Recalculate eclipse boundaries in unwrapped phase space - All eclipse detection and binning now happens in unwrapped phase space This eliminates the numerical discontinuity problem when eclipses cross the 0/1 phase boundary.
Replaces shift_bin_edges with _rewrap_to_original_phase method. - Add _rewrap_to_original_phase() to reverse unwrapping - Update calculate_bins() to use rewrapping with return_in_original_phase parameter - Perform binning in unwrapped space, optionally return in original space - Fix bin count checking logic (np.any instead of np.all) - Add minlength parameter to bincount for correct sizing This is a cleaner approach than the old shift_bin_edges which shifted bins so the rightmost edge = 1.0 and created shifted_phases in data dict.
- Update plot_binned_light_curve to use _rewrap_to_original_phase for eclipse boundaries - Update plot_unbinned_light_curve to rewrap phases and boundaries for display - Remove use_shifted_phases parameter from all methods: - find_minimum_flux_phase - find_secondary_minimum_phase - get_eclipse_boundaries - _find_eclipse_boundaries - _find_eclipse_boundary - _helper_secondary_minimum_mask - Simplify all methods to work only with unwrapped phases - Eclipse boundaries stored in unwrapped space, rewrapped for plotting
- Update helper_eclipse_detection to expect proper ordering for all eclipses after unwrapping - Simplify helper_find_eclipse_minima by using direct property access - Remove helper_shift_bins test helper (no longer needed) - Remove calls to shift_bin_edges and use_shifted_phases (deprecated) - Update test calls to pass None for wrapped parameter (kept for compatibility)
- Add tests for primary wrapped eclipse and both eclipses near boundaries - Update CLAUDE.md with phase unwrapping documentation - Fix duplicate bin edge issues by adding duplicates='drop' to pd.qcut calls - Add final deduplication step in calculate_bins to handle boundary duplicates - Allow graceful degradation tolerance (2%) for bin count differences - Skip pathological parameter combinations (high nbins + low fraction_in_eclipse) - Ensure unique bin edges before passing to scipy.stats.binned_statistic
- Format eclipsebin/binning.py and tests/test_eclipsing_binary_binner.py with black - Remove temporary verification scripts created during development - Add .venv/ to .gitignore to prevent accidental commits
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR introduces a phase unwrapping mechanism to the EclipsingBinaryBinner to resolve issues where eclipses crossing the phase boundary ([0, 1]) were incorrectly binned. By shifting
phases during initialization, we ensure that both primary and secondary eclipses are contiguous in phase space before any binning logic is applied.
Key Changes
unwrapped phase space.
Testing & Verification