feder-observatory / stellarphot

Stellar aperture photometry
https://stellarphot.readthedocs.io
BSD 3-Clause "New" or "Revised" License
4 stars 12 forks source link

Update comparison star notebook #364

Closed mwcraig closed 2 months ago

mwcraig commented 3 months ago

This is still a hot mess, but attempts to fix #183 by adding a source location widget to the comparison viewer and autosaving the aperture file.

Issues to be dealt with before merging:

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 94.02985% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.56%. Comparing base (290e646) to head (3fb5961). Report is 1 commits behind head on main.

:exclamation: Current head 3fb5961 differs from pull request most recent head d7fbfa6

Please upload reports for the commit d7fbfa6 to get more accurate results.

Files Patch % Lines
stellarphot/gui_tools/comparison_functions.py 90.69% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #364 +/- ## ========================================== + Coverage 76.48% 77.56% +1.08% ========================================== Files 27 27 Lines 3665 3629 -36 ========================================== + Hits 2803 2815 +12 + Misses 862 814 -48 ```

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

mwcraig commented 3 months ago

@Tanner728 @JuanCab this is still a work in progress but any feedback you have based on what is here so far is welcome....

Tanner728 commented 3 months ago

@mwcraig on this first look through I didn't notice anything wrong with the code although I don't know widgets that well. marked some minor typos but overall looks good. My one question is why you removed the TESS stuff, is it becoming too much work and causing issues? Also where is this new widget located? I didn't see it in the two notebooks.

mwcraig commented 3 months ago

@Tanner728 -- thanks for taking a look!

marked some minor typos but overall looks good.

Can you comment on those in github so I can fix them?

My one question is why you removed the TESS stuff, is it becoming too much work and causing issues?

I'd like to mostly move that into another widget, because the comparison viewer is pretty complicated even without it. Though it doesn't exist yet, there would be some sort of "generate your TESS stuff" widget for generating those things. I have kept the TESS code in a separate branch.

Also where is this new widget located?

It hasn't been added yet, will do that...

mwcraig commented 3 months ago

While this could still be beautified, I think it is ready for review. Further beautification can wait until a later pull request

mwcraig commented 3 months ago

FYI, I think the codecov report on this PR i incorrect -- all but one of the lines marked as uncovered in the patch are now covered with commit d4fd8e0.

Tanner728 commented 3 months ago

@Tanner728 -- thanks for taking a look!

marked some minor typos but overall looks good.

Can you comment on those in github so I can fix them?

My one question is why you removed the TESS stuff, is it becoming too much work and causing issues?

I'd like to mostly move that into another widget, because the comparison viewer is pretty complicated even without it. Though it doesn't exist yet, there would be some sort of "generate your TESS stuff" widget for generating those things. I have kept the TESS code in a separate branch.

Also where is this new widget located?

It hasn't been added yet, will do that...

Sorry had forgotten to hit submit on the review. I will review again tonight after work.

mwcraig commented 3 months ago

I think this is go to go -- thanks for catching the types @Tanner728 and for the review/off-line discussion @JuanCab

mwcraig commented 3 months ago

Thanks @JuanCab -- I opened #368 to keep track of the symbol color issue and addressed the rest of your comments, I think.