Add corrections

[AkashA98]

This address the issue of flux/astrometric corrections to the raw/cropped images. Fixes #20 , #4 , #7 .

[AkashA98]

I am happy if we wait before merging this branch into dev. I am running this code currently on VAST data right away anyway.

[mubdi]

Only reviewed it for code standardization purposes. Will need a bunch of doc strings and a bit of reorg here.

[mubdi]

@AkashA98 -- @hansenjiang's reorganization has been merged into dev. If you can pull to rebase to use that as an example, that'd be great! Thanks!

[ddobie]

I've gone through and added some preliminary comments on specific parts of the code. General comments:

1. As Mubdi has already noted, a lot of this needs more documentation - right now it's very opaque and I'm struggling to understand what a lot of it is doing
2. Needs to be written in a way such that you can integrate this step into a larger pipeline rather than running it standalone. That means splitting a lot of the larger functions into smaller ones and moving everything but the bare minimum out of the CLI script.
3. I think we want to pull the reference catalogues from somewhere else or even just include them in the package (e.g. combine them into a single parquet file) - I'm not sure the default directory you're pointing to even has all of the VAST fields?

[ddobie]

Split into a filter_sources function

[ddobie]

Also need to filter on:

• no nearby sources (use cat.match_to_catalog_sky(cat, nthneighbor=2) or similar
• compact source (@dlakaplan do you think it's better to use int/peak ratio or to compare the source FWHM to the PSF FWHM?)
• Some sort of flux or S/N cut - Maybe S/N > 20?

[AkashA98]

updated the filter function to select isolated (neighbor>1'), point (peak/integrated<1.5), and bright (snr>20) sources.

[ddobie]

One other thing - you need to write the corrections you're applying to the fits headers and as parameters in the selavy catalogues.

[ddobie]

Another general comment - you should use f-strings throughout. e.g. instead of

logger.debug("Using crossmatch radius: %s.", radius)

you can use

logger.debug(f"Using crossmatch radius: {radius}.")

[ddobie]

@AkashA98 I suspect that something has gone wrong with the merge and rebase - there's a bunch of commits in this pull from @hansenjiang that I don't think should be in here.

How did you actually do it? Should just be something like

git checkout dev
git pull
git checkout add_corrections
git merge dev

[AkashA98]

@AkashA98 I suspect that something has gone wrong with the merge and rebase - there's a bunch of commits in this pull from @hansenjiang that I don't think should be in here.

How did you actually do it? Should just be something like

git checkout dev
git pull
git checkout add_corrections
git merge dev

I did not merge my branch into dev. As @mubdi suggested, I just rebased it to an updated version of dev branch after @hansenjiang pushed his commits (I did overwrite a couple of conflicts from @hansenjiang to update the correction-related code). I am relatively new to this rebasing stuff, but I thought that the residual commits from @hansenjiang that were not conflicting with mine will remain since I rebased, but maybe I am wrong. Can @mubdi / @hansenjiang confirm if it is the case?

[mubdi]

@AkashA98 I suspect that something has gone wrong with the merge and rebase - there's a bunch of commits in this pull from @hansenjiang that I don't think should be in here.
How did you actually do it? Should just be something like

git checkout dev
git pull
git checkout add_corrections
git merge dev

I did not merge my branch into dev. As @mubdi suggested, I just rebased it to an updated version of dev branch after @hansenjiang pushed his commits (I did overwrite a couple of conflicts from @hansenjiang to update the correction-related code). I am relatively new to this rebasing stuff, but I thought that the residual commits from @hansenjiang that were not conflicting with mine will remain since I rebased, but maybe I am wrong. Can @mubdi / @hansenjiang confirm if it is the case?

Yep, this is correct -- there was an earlier dev PR that @AkashA98 rebased against. The changes you see made by @hansenjiang were from that commit.

[AkashA98]

One other thing - you need to write the corrections you're applying to the fits headers and as parameters in the selavy catalogues.

Done

[ddobie]

@AkashA98 I suspect that something has gone wrong with the merge and rebase - there's a bunch of commits in this pull from @hansenjiang that I don't think should be in here.
How did you actually do it? Should just be something like

git checkout dev
git pull
git checkout add_corrections
git merge dev

I did not merge my branch into dev. As @mubdi suggested, I just rebased it to an updated version of dev branch after @hansenjiang pushed his commits (I did overwrite a couple of conflicts from @hansenjiang to update the correction-related code). I am relatively new to this rebasing stuff, but I thought that the residual commits from @hansenjiang that were not conflicting with mine will remain since I rebased, but maybe I am wrong. Can @mubdi / @hansenjiang confirm if it is the case?

Yep, this is correct -- there was an earlier dev PR that @AkashA98 rebased against. The changes you see made by @hansenjiang were from that commit.

But those changes should already be on dev shouldn't they? And hence shouldn't be showing as changed files on this PR?

The way I've always done things (which might not be best practice) is to merge the latest version of dev into your own branch and then submit a PR from your branch to dev. Just concerned that Akash might have rebased against an older version of dev, or a different branch, and hence this PR is actually undoing changes

[AkashA98]

@AkashA98 I suspect that something has gone wrong with the merge and rebase - there's a bunch of commits in this pull from @hansenjiang that I don't think should be in here.
How did you actually do it? Should just be something like

git checkout dev
git pull
git checkout add_corrections
git merge dev

I did not merge my branch into dev. As @mubdi suggested, I just rebased it to an updated version of dev branch after @hansenjiang pushed his commits (I did overwrite a couple of conflicts from @hansenjiang to update the correction-related code). I am relatively new to this rebasing stuff, but I thought that the residual commits from @hansenjiang that were not conflicting with mine will remain since I rebased, but maybe I am wrong. Can @mubdi / @hansenjiang confirm if it is the case?

Yep, this is correct -- there was an earlier dev PR that @AkashA98 rebased against. The changes you see made by @hansenjiang were from that commit.

But those changes should already be on dev shouldn't they? And hence shouldn't be showing as changed files on this PR?

The way I've always done things (which might not be best practice) is to merge the latest version of dev into your own branch and then submit a PR from your branch to dev. Just concerned that Akash might have rebased against an older version of dev, or a different branch, and hence this PR is actually undoing changes

No, the dev branch I am rebasing onto is the one that Hansen already pushed the changes to. So, when I try to rebase it, it adds all those commits to my branch that Hansen committed to dev and they are showing up here

[AkashA98]

Another general comment - you should use f-strings throughout. e.g. instead of

logger.debug("Using crossmatch radius: %s.", radius)

you can use

logger.debug(f"Using crossmatch radius: {radius}.")

All such code is now corrected.

[AkashA98]

ok, @mubdi , @ddobie , @hansenjiang . I have tested the code with a couple of epochs of data and it works fine. I still have to come up with a rigorous set of tests for this, but this PR right now is good to go (modulo the init file which I rebased from Hansen's dev branch)

[mubdi]

GTG!

All now addressed.