Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fix and remove risky cropping code #748

Merged
merged 4 commits into from
May 9, 2023
Merged

Conversation

jeff-regier
Copy link
Contributor

@jeff-regier jeff-regier commented May 8, 2023

This PR fixes the star flux bug and includes a newly pretrained sdss.pt model in data/pretrained_models/.

@aakashdp6548 Please lmk if this fixes the issue with the test you are writing.

@XinyueLi1012 Please lmk if you can increase the threshold in your test following this PR.

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #748 (ef3b354) into master (b69b6c5) will increase coverage by 1.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
+ Coverage   70.06%   71.36%   +1.30%     
==========================================
  Files          13       13              
  Lines        1326     1299      -27     
==========================================
- Hits          929      927       -2     
+ Misses        397      372      -25     
Flag Coverage Δ
unittests 71.36% <100.00%> (+1.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
bliss/catalog.py 80.09% <100.00%> (+8.60%) ⬆️
bliss/unconstrained_dists.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jeff-regier jeff-regier marked this pull request as draft May 8, 2023 20:52
@jeff-regier jeff-regier marked this pull request as ready for review May 9, 2023 13:02
@aakashdp6548
Copy link
Collaborator

aakashdp6548 commented May 9, 2023

Looks good to me. The star_fluxes values are more reasonable, and taking the top 100 gives me pretty good results:
cat_comparison_top100

Also fwiw, the number of sources is also a little bit better with the new model - 197 predicted compared to 200 in SDSS.

@XinyueLi1012
Copy link
Collaborator

The value for flux_diff / flux_sum is still around 0.15 using 100 as cut off for the fluxes value in the reconstruction image. I use [160:320] cropped sdss image because there is no fluxes bigger than 100 in [0:160]:

True image (image minus background)
image

Reconstructed image
image

But the reconstructed image looks good to me.

@jeff-regier jeff-regier merged commit 862a3a6 into master May 9, 2023
@jeff-regier jeff-regier deleted the jr/rescale-crop branch May 9, 2023 15:20
zhixiangteoh pushed a commit that referenced this pull request May 9, 2023
* don't crop FullCatalogs because it's ambigous where doing so rescales the coordinates

* fixed bad bug that I introduced rently

* added newly pretrained sdss encoder

* reference sdss.pt instead of crowded_field.pt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants