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

Notebook for using updated astrometry solutions #120

Merged
merged 12 commits into from
Dec 6, 2019

Conversation

cshanahan1
Copy link
Contributor

Added a notebook to the DrizzlePac subdirectory with how to access and use the updated astrometry solutions for wfc3 and acs data. This will only run with the latest version of stwcs (and drizzlepac as well I believe - to be safe I put the latest version as the minimum in the requirements).

@cshanahan1 cshanahan1 requested a review from eteq as a code owner December 3, 2019 06:30
@eteq
Copy link
Collaborator

eteq commented Dec 4, 2019

Hmm. The built notebook is here: https://144-138342879-gh.circle-artifacts.com/0/root/repo/notebooks/DrizzlePac/using_updated_astrometry_solutions/using_updated_astrometry_solutions.html

So it looks like it's not getting very far. The error is not terribly informative to me though. Do you think this might be because it's not an up-to-date enough version, @shannnnnyyy ?

Copy link
Collaborator

@eteq eteq left a comment

Choose a reason for hiding this comment

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

The content looks good except for my pretty minor style/word choice suggestions.

I notice there's a commit in there with some un-cleared outputs. Normally I'd say squash that out, but it looks like it's just a traceback, rather than a figure which makes the file a lot more awkward to deal with. So we'll let it fly this time 😉

However, the notebook isn't currently executing as per my comment above. That'll need to be fixed before we can merge this...

"cell_type": "markdown",
"metadata": {},
"source": [
"# Using updated astrometry solutions"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm finding this title a bit hard to fully understand the context. Perhaps "Choosing alternate astrometry solutions for drizzling"?

(If you make the change here, it's probably good to also rename the file and directory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context will usually be for drizzling, but not always so I think it might be best to keep it general. I'll brainstorm a title that makes more sense and gives more context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's ok with you I think i'll just leave this title for now, I can't think of anything better

Copy link

Choose a reason for hiding this comment

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

The idea was to keep it general, as I didn't want users to think this was something to ignore if they're not necessarily running drizzle. It's really applicable for any case where the WCS is used, as these are yielding different WCS's.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, point taken. Then we can leave it as is.

@eteq
Copy link
Collaborator

eteq commented Dec 5, 2019

I pushed up a few fixes since that was going to be faster than suggesting them (I'd already run the notebook locally), but I think this is now running correctly. It's still mysterious that the check box appeared even though the notebook was failing, but that's #121 now.

So this is probably good if @shannnnnyyy is ok with the changes I made.

@cshanahan1
Copy link
Contributor Author

@eteq everything looks good to me!

@eteq eteq merged commit 459fcc6 into spacetelescope:master Dec 6, 2019
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