Skip to content
This repository has been archived by the owner on Oct 21, 2024. It is now read-only.

Minor cleanup in compute_overlap #46

Merged
merged 4 commits into from
Apr 28, 2021

Conversation

lpsinger
Copy link
Contributor

This removes Healpy from the script and uses the high-level astropy-healpix API instead.

I don't understand what the compute_overlap function does. It looks like it is computing the area of largest overlap between the first 100 tiles and the rest of the tiles. However, it is multiply the number of pixels in the overlap by the pixel linear resolution in arc minutes, which does not make any dimensional sense. The number of pixels in the overlap is a measure of area.

This removes Healpy from the script and uses the high-level
astropy-healpix API instead.
@lpsinger lpsinger requested a review from mcoughlin April 28, 2021 04:47
@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #46 (fa0cb64) into main (8078b5d) will increase coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   26.63%   26.74%   +0.10%     
==========================================
  Files          28       28              
  Lines        1359     1350       -9     
==========================================
- Hits          362      361       -1     
+ Misses        997      989       -8     
Impacted Files Coverage Δ
dorado/scheduling/scripts/simsurvey.py 9.00% <0.00%> (-0.09%) ⬇️

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 8078b5d...fa0cb64. Read the comment docs.

@mcoughlin
Copy link
Collaborator

Looks like it is failing lint. This was just a quick cross-check for me, the whole function can be removed.

@lpsinger
Copy link
Contributor Author

Looks like it is failing lint.

I think the lint errors should be fixed now.

This was just a quick cross-check for me, the whole function can be removed.

It can? It's not dead code, it's toggled by the --doOverlap flag.

@mcoughlin
Copy link
Collaborator

You are computing field overlap elsewhere no?

@lpsinger
Copy link
Contributor Author

You are computing field overlap elsewhere no?

I'm not sure what you mean. The overlap is accounted for in the objective function, so I never directly calculate it. Do you need it for something?

@mcoughlin
Copy link
Collaborator

Basically if someone wants to check quickly what overlap their grid has, it is nice to be able to easily check.

@lpsinger
Copy link
Contributor Author

Basically if someone wants to check quickly what overlap their grid has, it is nice to be able to easily check.

I see. Well, I don't want to push you to remove it, if it is useful, but it does currently have nonsensical units, which should be fixed.

@mcoughlin
Copy link
Collaborator

Sounds good to me.

@lpsinger lpsinger merged commit 1e29f2f into nasa:main Apr 28, 2021
@lpsinger lpsinger deleted the compute-overlap-cleanup branch April 28, 2021 05:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants