eleanor icon indicating copy to clipboard operation
eleanor copied to clipboard

tf -> scipy

Open aditya-sengupta opened this issue 4 years ago • 6 comments

  • Takes out dependency on tensorflow, makes eleanor compatible with Python 3.8
  • Refactors targetdata.py:psf_lightcurve and expands functionality of models.py

aditya-sengupta avatar Sep 22 '20 09:09 aditya-sengupta

Hey, awesome! Let me play with it a little bit and try to break it, and assuming I can't I'll merge it in.

Have you done any benchmarking? How does the speed compare to the tf implementation?

benmontet avatar Sep 22 '20 09:09 benmontet

Codecov Report

Merging #198 into master will increase coverage by 2.30%. The diff coverage is 7.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   46.15%   48.45%   +2.30%     
==========================================
  Files          20       20              
  Lines        2247     2132     -115     
==========================================
- Hits         1037     1033       -4     
+ Misses       1210     1099     -111     
Impacted Files Coverage Δ
eleanor/models.py 0.00% <0.00%> (ø)
eleanor/targetdata.py 53.92% <7.31%> (+4.88%) :arrow_up:
eleanor/source.py 49.59% <75.00%> (+0.20%) :arrow_up:
eleanor/mast.py 88.29% <100.00%> (ø)
eleanor/visualize.py 13.79% <0.00%> (+1.97%) :arrow_up:
eleanor/update.py 84.18% <0.00%> (+5.81%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2090bc7...d0eac4b. Read the comment docs.

codecov[bot] avatar Sep 22 '20 09:09 codecov[bot]

Detailed this in an email just now: speed seems about comparable/maybe a little bit slower than tf (unclear how specific to my machine this is). This can be sped up by model updates/the Pytorch upgrade that aren't in this PR but should hopefully be coming soon!

aditya-sengupta avatar Sep 22 '20 09:09 aditya-sengupta

Hey,

The code looks great! I really appreciate your efforts here.

In testing I'm running in to some issues which are probably due to issues around the initialisation of parameters. I want to make sure the results look the same on your end and my end which will help isolate where the issue is and how to sort it. Can you send me the output of the following from your end?

> star = eleanor.Source(tic=38846515, sector=1)
> data = eleanor.TargetData(star, do_psf=True)
> q = data.quality == 0
> plt.plot(data.time[q], data.psf_flux[q], '.')

I see the following, which is very much not what the star is doing in reality! I'm reasonably confident this is an issue with the initialisation of parameters, with a bad initial guess leading to the minimiser finding a local rather than the global minimum.

Assuming you see the same thing I'll try to trace it through to see if I can find more flexibly sensible starting conditions and I have a feeling everything will go fine from there.

image

benmontet avatar Sep 29 '20 08:09 benmontet

Yes, I see a similar result, although I had to delete my earlier cached file and rerun.

[image: image.png]

Also, I ran into a quick bug around astropy units while I was running this

  • I think in a recent update they may have enforced unit-aware data processing, so the TPFs for me previously carried units of electron/s, but not this time. This is why I had the quick stopgap of dividing by (u.electron / u.s) (targetdata.py:860, 866, 869) but ideally we could have some slightly more sophisticated processing, i.e. rewriting downstream analysis to work with units and/or do steps differently for the unitless vs. unit-aware versions.

On Tue, Sep 29, 2020 at 1:45 AM benmontet [email protected] wrote:

Hey,

The code looks great! I really appreciate your efforts here.

In testing I'm running in to some issues which are probably due to issues around the initialisation of parameters. I want to make sure the results look the same on your end and my end which will help isolate where the issue is and how to sort it. Can you send me the output of the following from your end?

star = eleanor.Source(tic=38846515, sector=1) data = eleanor.TargetData(star, do_psf=True) q = data.quality == 0 plt.plot(data.time[q], data.psf_flux[q], '.')

I see the following, which is very much not what the star is doing in reality! I'm reasonably confident this is an issue with the initialisation of parameters, with a bad initial guess leading to the minimiser finding a local rather than the global minimum.

Assuming you see the same thing I'll try to trace it through to see if I can find more flexibly sensible starting conditions and I have a feeling everything will go fine from there.

[image: image] https://user-images.githubusercontent.com/1314530/94534271-862d0200-0283-11eb-925e-44b31939db44.png

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/afeinstein20/eleanor/pull/198#issuecomment-700547618, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIK476TS5H5E63NYBIC3MJ3SIGNDBANCNFSM4RVRRWSQ .

aditya-sengupta avatar Sep 29 '20 15:09 aditya-sengupta

Resolved the aforementioned unit issue

aditya-sengupta avatar Oct 04 '20 00:10 aditya-sengupta