-
Notifications
You must be signed in to change notification settings - Fork 41
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
Explore by group type #1000
Explore by group type #1000
Conversation
Hello @BradleySappington, Thank you for updating !
Comment last updated at 2022-07-26 13:18:08 UTC |
@mfixstsci ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @BradleySappington this looks good to me, thanks for the changes. Just have a couple of questions here and then we can merge.
@@ -32,7 +32,6 @@ | |||
import tempfile | |||
|
|||
from astropy.io import fits | |||
from astropy.table import Table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we not using Table
in this module @BradleySappington?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, my linter told me it was imported and unused. Doing minor code cleanups along the way. There are more cleanups/errors/bugs in this file that need to be done, but they affect the logic directly so I didn't want to put them in this code request, they should be done separately.
'base_url': get_base_url(), | ||
'form': form} | ||
|
||
return render(request, template, context) | ||
|
||
|
||
def explore_image_ajax(request, inst, file_root, filetype, scaling="log", low_lim=None, high_lim=None, rewrite=False): | ||
def explore_image_ajax(request, inst, file_root, filetype, scaling="log", low_lim=None, high_lim=None, ext_name="SCI", rewrite=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we always expect there to be a science extension available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with @bhilbert4 who confirmed before adding this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally this looked good on the test server.
No description provided.