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

Dithering utils - WIP #735

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open

Dithering utils - WIP #735

wants to merge 35 commits into from

Conversation

wtgee
Copy link
Member

@wtgee wtgee commented Nov 23, 2018

These are pulled from https://github.com/AstroHuntsman/huntsman-pocs

Besides some slight code cleanup the only difference is to specify a default pattern_offset of 30 arcminutes.

I've also removed the plotting options from the function itself. added back in.

Note that these are placed in the utils folder and are not yet a part of normal operations.

Part of #732

@wtgee wtgee mentioned this pull request Nov 23, 2018
13 tasks
@codecov
Copy link

codecov bot commented Nov 23, 2018

Codecov Report

Merging #735 into develop will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #735     +/-   ##
==========================================
+ Coverage    80.58%   80.79%   +0.2%     
==========================================
  Files           61       62      +1     
  Lines         5188     5243     +55     
  Branches       715      722      +7     
==========================================
+ Hits          4181     4236     +55     
  Misses         818      818             
  Partials       189      189
Impacted Files Coverage Δ
pocs/utils/dither.py 100% <100%> (ø)

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 c0941fb...7d41d0e. Read the comment docs.

Copy link
Collaborator

@AnthonyHorton AnthonyHorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks familiar! Made a few picky suggestions to improve docs, readability.

pocs/utils/dither.py Outdated Show resolved Hide resolved
pocs/utils/dither.py Outdated Show resolved Hide resolved
pocs/utils/dither.py Outdated Show resolved Hide resolved
@wtgee wtgee added the One of Many PRs PR is part of a multi-part sequence of PRs label Nov 23, 2018
@wtgee
Copy link
Member Author

wtgee commented Nov 24, 2018

@AnthonyHorton I've added the plotting options back although changed slightly. Mostly I'm not using pyplot but instead generating an actual Figure and returning that. It is up to the user to display/save.

I also simplified it a bit so the only param to the plotting function is the generated positions. This means it does not show the given offsets as part of the legend or title. Since the figure is returned to the user these could be added in after the fact if desired.

I've also added testing via pytest-mpl

Note that this is just to make things work. The image should be exactly
the same but my feeling is that the mpl backend or something is causing
a difference. The normal way to deal with this would be to upload the failed
test to some location where you could inspect the images and see why
different. Unfortunately the main way to do that is via the travis aretfacts,
which don't work for pull requests.

So, long story short, I'm not sure this is effectively testing this.
@wtgee
Copy link
Member Author

wtgee commented Nov 24, 2018

I've wasted too much time on the dithering plot but did finally get the image upload of the failed images to work. Here's the output (baseline, created, diff). Needless to say I'm pretty frustrated. Note that this has the remove_text=True option so it should be ignoring the text but I guess that is only the title. Apparently there is some minor difference in the font style that is rendering a 13% rms on the image diff. 😠

baseline-test_plot_dither

test_plot_dither

test_plot_dither-failed-diff

wtgee added a commit to wtgee/POCS that referenced this pull request Nov 25, 2018
Depends on panoptes#735

* Rearrange to support mulitple types of `Observation`s.
* Add a `DitheredObservation` (coming from AstroHuntsman/huntsman-pocs).
@@ -91,3 +91,5 @@ cache:
- $PANDIR/astrometry/
after_success:
- bash <(curl -s https://codecov.io/bash)
after_failure:
- bash scripts/testing/failed-images-upload.sh ${POCS}/test_images/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamessynge when you get a chance I would appreciate your thoughts on this.

@wtgee wtgee removed the request for review from jamessynge December 1, 2018 06:49
@wtgee wtgee changed the title Dithering utils Dithering utils - WIP Dec 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
One of Many PRs PR is part of a multi-part sequence of PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants