kevin218 / Eureka

Eureka! is a data reduction and analysis pipeline intended for time-series observations with JWST.
https://eurekadocs.readthedocs.io/
MIT License
56 stars 44 forks source link

Upgrade binData() --> binData_time() #584

Closed hdiamondlowe closed 5 months ago

hdiamondlowe commented 8 months ago

I have added a function called binData_time to util.py. This function uses the binned_statistic function from scipy.stats to better handle the binning of time series data. A pdf of an example of the improvement can be found in Issue #583.

I have updated plots_s5.py to use binData_time instead of binData, but I have only tested this for MIRI aperture photometry. I do not know if it will break with other instruments/modes.

To make this change as noninvasive as possible, I have kept the binData function in place so that calls to it from other Stages are not disrupted.

hdiamondlowe commented 8 months ago

Hi @taylorbell57, today I found a bug in my binning fix where nans/infs were not being handled appropriately. I've fixed it in my hdl_binning branch. I'm not sure if you'd like me to do anything about updating the PR or something like that, but I just wanted to let you know.

taylorbell57 commented 8 months ago

Hi @taylorbell57, today I found a bug in my binning fix where nans/infs were not being handled appropriately. I've fixed it in my hdl_binning branch. I'm not sure if you'd like me to do anything about updating the PR or something like that, but I just wanted to let you know.

All your pushed edits automatically got included in this PR, so nothing needed from you at this moment. I think those issues with NaNs were why I ultimately chose not to use that function "for the time being" while intending to come back and fix it later. Thanks for working that all out! I'll try to review this within a couple of days

taylorbell57 commented 7 months ago

I made some small tweaks to ensure flake8 compliance, but there is some issue with the MIRI test that I can't figure out (don't worry about the failed automated pytest and flake8 tests above - I forgot those only work for us devs and fail when someone outside the team makes a PR). I'll circle back around to this later, but the error that comes up in the automated testing is:

>       s5_meta = s5.fitlc(meta.eventlabel, ecf_path=ecf_path, s4_meta=s4_meta)

/home/taylor/Eureka2/tests/test_MIRI.py:87: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/home/taylor/Eureka2/src/eureka/S5_lightcurve_fitting/s5_fit.py:290: in fitlc
    meta, params = fit_channel(meta, time, flux, 0, flux_err,
/home/taylor/Eureka2/src/eureka/S5_lightcurve_fitting/s5_fit.py:856: in fit_channel
    lc_model.fit(model, meta, log, fitter='dynesty')
/home/taylor/Eureka2/src/eureka/S5_lightcurve_fitting/lightcurve.py:179: in fit
    fit_model = self.fitter_func(self, model, meta, log, **kwargs)
/home/taylor/Eureka2/src/eureka/S5_lightcurve_fitting/fitters.py:900: in dynestyfitter
    plots.plot_phase_variations(lc, model, meta, fitter='dynesty')
/home/taylor/Eureka2/src/eureka/S5_lightcurve_fitting/plots_s5.py:254: in plot_phase_variations
    ax.set_ylim(-6*sigma, max_astro+6*sigma)
/home/taylor/miniconda3/envs/eureka_starry/lib/python3.9/site-packages/matplotlib/axes/_base.py:3898: in set_ylim
    return self.yaxis._set_lim(bottom, top, emit=emit, auto=auto)
/home/taylor/miniconda3/envs/eureka_starry/lib/python3.9/site-packages/matplotlib/axis.py:1210: in _set_lim
    v0 = self.axes._validate_converted_limits(v0, self.convert_units)
/home/taylor/miniconda3/envs/eureka_starry/lib/python3.9/site-packages/matplotlib/axes/_base.py:3585: ValueError: Axis limits cannot be NaN or Inf

My guess is that something in the new function breaks for MIRI's rotated spectra, but I'll investigate more later.

taylorbell57 commented 5 months ago

Strange, that latest commit of mine lets the tests pass locally. Perhaps its a sporadic issue, but I really hope not... I'll try something else

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5be7e90) 57.72% compared to head (f087460) 57.69%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #584 +/- ## ========================================== - Coverage 57.72% 57.69% -0.03% ========================================== Files 96 96 Lines 11660 11669 +9 ========================================== + Hits 6731 6733 +2 - Misses 4929 4936 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

taylorbell57 commented 5 months ago

Huh, so it seems that the GitHub pytest runs were only using the code merged into origin/hdl_binning and not hdiamondlowe/hdl_binning which is very strange since the commits to hdiamondlowe/hdl_binning are the only ones that show up here... Very strange and buggy behaviour, and something we'll need to be careful about in the future