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

Simbad/NED async cone search #336

Merged
merged 8 commits into from
Sep 21, 2020
Merged

Simbad/NED async cone search #336

merged 8 commits into from
Sep 21, 2020

Conversation

marxide
Copy link
Contributor

@marxide marxide commented Sep 14, 2020

Added API endpoints for SIMBAD and NED cone searches via astroquery.
Added a DataTable for SIMBAD and NED cone search results on the source detail page. These use the new API endpoints so they do not hold up the page load and are rendered when ready.
The DataTables are collapsed by default.
The cone searches have a radius of 1 arcmin.
Closes #331

@github-actions github-actions bot added this to In progress in Pipeline Backlog Sep 14, 2020
@marxide marxide linked an issue Sep 14, 2020 that may be closed by this pull request
@ajstewart
Copy link
Contributor

The content looks good but I think:

  • Should be expanded by default, I don't think we should be adding extra clicks.
  • While the SIMBAD and NED tabs are nice and neat it again introduces an extra click, and not ideal to look at both at the same time. Two separate tables next to each other might be better.

On a side note, this is a separate issue, but the page is becoming a bit too vertical for desktop devices, it would be good if the results were at the top along side the details and Aladin. In my opinion we should be aiming to represent most of the information that you'd normally want without having to scroll too much or click things. We know that our use case is that if someone clicks a source they want to know more about it, this should be as convenient as possible.

@marxide
Copy link
Contributor Author

marxide commented Sep 15, 2020

OK, try this.

I moved the results to the top row and combined them into a single table. The source of the result is appended to the object name. A separate column would be preferable but horizontal space is getting tight for smaller monitors. I set the breakpoints so the Aladin Lite viewer moves underneath if the window gets too narrow.

@ajstewart
Copy link
Contributor

Yep this looks great, though I'm getting a bit of overspill when the table height changes, see Aladin is overlapping with the light curve card:
Screen Shot 2020-09-16 at 14 17 22
can the height be fixed the card height rather than the table?

I'm starting to think that we don't need the lightcurve to spread the entire width - but I know you have moved this to Bokeh in the other PR? As you could put the Aladin or tables below next to the lightcurve, and have a 2 x 2 setup. But then you'll probably be adding the two epoch graph?

Technically when the adjacent card to the parent changes size.
@marxide
Copy link
Contributor Author

marxide commented Sep 16, 2020

Thanks, I didn't catch the case when the table shrinks when the last page isn't full.

The Aladin overflow is tricky. The cards are set to be of equal heights, but the internal Aladin Lite elements don't play well with others! The parent card element is being resized properly, but the canvas element that Aladin Lite puts in there is only updated when the window is resized, not the parent element (it listens for window.resize). Unfortunately, there isn't a widely supported way of listening for element size changes without polling. There is ResizeObserver but it's fairly new https://caniuse.com/resizeobserver.

I found the function Aladin calls when the window is resized, so I've put in another listener on the table, this time on draw.dt so Aladin will resize the canvas elements whenever the table is drawn (e.g. after paging).

@marxide
Copy link
Contributor Author

marxide commented Sep 16, 2020

Yes, some rearrangement will be required after merging #305. The Bokeh lightcurve ends up appearing more compact, even with the 2-epoch graph, and I added the 2-epoch metrics to the details card so that now needs more horizontal space.

Screen Shot 2020-09-16 at 11 41 06 AM

I'd prefer to not have the backend features of this PR held up by waiting for the benchmarks before merging #305. If all that is left is layout tweaking, I suggest we merge this and continue to iterate on that in #305.

ajstewart
ajstewart previously approved these changes Sep 17, 2020
Copy link
Contributor

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

I agree, everything looks good to me here. Only comment is to perhaps change the radius search on the SIMBAD link as we're doing changes anyway.

templates/source_detail.html Outdated Show resolved Hide resolved
Pipeline Backlog automation moved this from In progress to Reviewer approved Sep 17, 2020
Pipeline Backlog automation moved this from Reviewer approved to Review in progress Sep 17, 2020
@marxide
Copy link
Contributor Author

marxide commented Sep 17, 2020

Good point. Done.

Pipeline Backlog automation moved this from Review in progress to Reviewer approved Sep 17, 2020
ajstewart
ajstewart previously approved these changes Sep 17, 2020
@ajstewart
Copy link
Contributor

I think we can also merge this now before the first release (@srggrs).

@srggrs
Copy link
Contributor

srggrs commented Sep 18, 2020

I think we can also merge this now before the first release (@srggrs).

ok I'll review it. Btw there some conflicts....

Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

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

I think everything looks good. I personally prefer writing operations like this:

df_with_operations = (
    df_raw[['col1', 'col2']]# selection line
    .apply( . . . )# apply function line
    .rename( . . . )# rename operation
    . . . # add other operations for each line
)

so you end up with a line for each operation and the code would be more clear to me than:

df_with_oprations = df[
   ['col1', 'col2']
].apply( . . . ) 

Anyhow you don't need to follow this.

In any case there are few conflicts that need to be resolved before merging is permitted.

pipeline/views.py Show resolved Hide resolved
pipeline/views.py Show resolved Hide resolved
pipeline/views.py Show resolved Hide resolved
Pipeline Backlog automation moved this from Reviewer approved to Review in progress Sep 18, 2020
@marxide marxide requested a review from srggrs September 18, 2020 16:46
Pipeline Backlog automation moved this from Review in progress to Reviewer approved Sep 20, 2020
@marxide marxide merged commit 2947d2a into master Sep 21, 2020
Pipeline Backlog automation moved this from Reviewer approved to Done Sep 21, 2020
@marxide marxide deleted the simbad-async branch September 21, 2020 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Embed SIMBAD/NED searches on the source detail page
3 participants