select sources by table #2
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| "sphinx-gallery", | ||
| "pydata-sphinx-theme" | ||
| "pydata-sphinx-theme", | ||
| "pandas" |
There was a problem hiding this comment.
note: we had to add pandas here since we are converting the astropy table to pandas in order to get the json safe dictionary of user defined keys to search on. This is the best way I've found to do this, but I'm open to any suggestions
| ), | ||
| ); | ||
| }) | ||
| .filter((n) => n.length); |
There was a problem hiding this comment.
note: this filter statement is here to remove any empty lists that may be returned from the first filter statement. If there are multiple catalogs, and the key only matches on some of them, it will return something like the following
[
[], // empty list
[ ... ] // list full of sources
]
The empty list needs to be removed for the selectObjects call to function properly
There was a problem hiding this comment.
I feel like there is already enough happening with all the nested functions in the map() line. I would just do this last filter() on a new line preceded by a comment.
| let sourcesByCatalog = this.aladin.view.catalogs | ||
| .map((cat) => { | ||
| if (!cat.isShowing) { | ||
| return; |
There was a problem hiding this comment.
Might be clearer to explicitly return [].
| return; | ||
| } | ||
| return cat.getSources().filter((source) => | ||
| sources.some((s) => |
There was a problem hiding this comment.
I found the name sources confusing here since they are very different from the getSources() and the source being filtered. Perhaps rename to indicate that they are the sources to be selected. This could tie in with a succinct comment or two here that clarify how the nested functions are filtering the lists.
I'd also be tempted to store the filter() result in a local variable and return that on a separate line. This could help the readability a little and would make any interactive debugging easier.
| ), | ||
| ); | ||
| }) | ||
| .filter((n) => n.length); |
There was a problem hiding this comment.
I feel like there is already enough happening with all the nested functions in the map() line. I would just do this last filter() on a new line preceded by a comment.
What is changing?
Enabling selection of sources in aladin from an astropy table by unique key(s)
Why is this change needed?
This change enables both CST-171 and CST-179 by enabling selection of sources defined in an astropy table, and by supporting matching by a user defined set of keys.
This will enable automation of notebooks in a variety of ways. For our initial use case, this will enable importing and exporting data to and from ipyaladin and jdaviz.
How was this tested?
The attached notebook was used to manually test these changes. In the future it would be helpful to have an automated test suite that could be used to test a variety of edge cases.