DM-13624 Create HiPS survey loading framework #558

cwang2016 · 2018-03-08T02:13:18Z

The development includes,

Implement hips sources framework with search processor to search and combine available HiPS data from various sources, including CDS MocServer, IRSA, LSST, and others.
In current release, only CDS and IRSA sources provide the list of HiPS. For CDS, Firefly gets list of HiPS by sending query as shown in http://aladin.u-strasbg.fr/hips/list. For IRSA, Firefly prepares a file, irsa-hips-master-table.csv, to define the available HiPS.
Based on the implemented framework, a HiPS master list search query is sent to the server and returned with a HiPS master table containing the information of all available HiPS from various sources.
In this release, a filter which forms a table containing popular HiPS by modifying HiPS master table is added. In near future(https://jira.lsstcorp.org/browse/DM-13172), filter for making popular HiPS list will be made to be configurable by the application.

test:

do catalog search, and click ‘Find HiPS Plot’ to get HiPS master table or popular table.
do image search by providing URL or clicking one HiPS from the HiPS master or popular table. The HiPS table highlight which reflects the image of the active plot is updated dynamically as the active plot is changed among multiple images.

ejoliet · 2018-03-08T23:22:44Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/IrsaHiPSListSource.java

+    }
+
+
+    // this is temporary solution for getting HiPS from IRSA


I just wonder why is this temporary? Is there a plan for something else?

If IRSA were to deploy some sort of hipslist service then that would be the final solution.

Is there some way to get HiPS from IRSA? For this release to make HiPS source framework to aggregate HiPS from various sources, a hard code csv file is created and meant to contain the info of HiPS from IRSA.

No, IRSA doesn't have a registry service where you can get the list and i don't think is there any plan to do that in the (near) future so the solution you implemented is probably more definitive than temporary ;-)

loitly

I've made many comments. At least a few of those should be changed before merging.
Also:
From the UI testing, I think you should enable filtering on default for non-popular HiPS. There's enough rows where filtering can be helpful.

loitly · 2018-03-09T16:00:00Z

src/firefly/java/edu/caltech/ipac/firefly/server/rpc/VisServerCommands.java

+        public String doCommand(SrvParam sp)  throws Exception {
+            TableServerRequest tsr = sp.getTableServerRequest();
+            DataGroupPart dgp = new SearchManager().getDataGroup(tsr);
+            JSONObject json = JsonTableUtil.toJsonTableModel(dgp, tsr);


This is the exact same as a edu.caltech.ipac.firefly.server.query.SearchServerCommands.TableSearch.
When getting a TableModel from a SearchProcessor, you should use one of the fetch functions from TablesCntlr.js. No need to add a new command for every new SearchProcessor.

yes I will remove it. This was for some request created earlier. Now actually 'TableSearch' is used for HiPS search.

loitly · 2018-03-09T16:12:57Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/HiPSMasterList.java

+        {@ParamDoc(name = ServerParams.DATA_TYPE, desc = "types of HiPS data to search"),
+         @ParamDoc(name = "hipsSource", desc = "HiPS sources"),
+         @ParamDoc(name = "sortOrder", desc = "HiPS order, source based"),
+         @ParamDoc(name = "tblId", desc = "table Id")


Sorting and tblId is an existing concept of SearchProcessor. Is this different from that? Is it needed?

sortOrder has its meaning for this processor.

loitly · 2018-03-09T16:33:10Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/CDSHiPSListSource.java

@@ -0,0 +1,139 @@
+package edu.caltech.ipac.firefly.server.visualize.hipslists;


Not a big deal, but this package named hipslist. Is there a plan to have other hips related code immediately under visualize? If not, shouldn't it just be hips?
Also, naming it hips will allow for other hips related code to be in this package.

yes, change it to be 'hips'

loitly · 2018-03-09T16:39:04Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/HiPSMasterList.java

+import edu.caltech.ipac.firefly.server.visualize.hipslists.HiPSMasterListEntry.PARAMS;
+
+import org.json.simple.JSONArray;
+import org.json.simple.JSONObject;


Please clean up imports. The reference to JSON caught my eyes, but there are a couple more above that's less obvious.

loitly · 2018-03-09T16:59:38Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/HiPSMasterList.java

+            if (!cmeta.containsKey(key)) {
+                dg.addAttribute(key, meta.getAttribute(key));
+            }
+        }


This is not needed. prepareTableMeta is called within the framework. Individual processor should override prepareTableMeta when needed. You may have this confused because I gave you the wrapper code which is a piece of code used to bridge the old ipac table based to the new db based logic.

loitly · 2018-03-09T17:14:53Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/HiPSMasterList.java

+        return file;
+    }
+
+    private static Object strToObject(String s, Class c) {


Maybe you can use DataType#convertStringToData instead?

loitly · 2018-03-09T17:20:39Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/HiPSMasterList.java

+                Object val = keyVal == null ? null : strToObject(keyVal, cols.get(i).getDataType());
+
+                row.setDataElement(cols.get(i), val);
+            }


use convertStringToData like this?

for (DataType col : cols) { String val = mapInfo.get(col.getKeyName()); row.setDataElement(col, col.convertStringToData(val)); }

great, thanks.

loitly · 2018-03-09T17:26:25Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/LsstHiPSListSource.java

+/**
+ * Created by cwang on 2/27/18.
+ */
+public class LsstHiPSListSource implements HiPSMasterListSourceType {


@robyww Just throwing out the idea. Does not have to be done now.
This is the 2nd ListSourceType interface we have now. The 1st is ImageMasterDataSourceType.
If we plan to grow this interface, we should refactor it. There's a lot of duplicates between the 2.

loitly · 2018-03-09T18:17:00Z

src/firefly/js/ui/HiPSSurveyListDisplay.jsx

@@ -287,30 +264,52 @@ function fieldReducer(hipsUrl) {
                tooltip: 'display popular HiPS'
            }};
        }
-        return inFields;
+        return Object.assign({}, inFields);


You are returning a new Object all the time. Wouldn't that cause the FieldGroupCntlr to update it state?
I am not sure, but if that's the case, then you should only return a new object when you actually modify it.

Good catch. It is leftover from some code change. fixed.

loitly · 2018-03-09T18:20:37Z

src/firefly/js/visualize/HiPSCntlr.js

-import {flux} from '../Firefly.js';
-import {get, isArray, isEmpty, set} from 'lodash';
-import {fetchUrl, parseUrl} from '../util/WebUtil.js';
+import {get, isArray, cloneDeep, set, isFunction, isUndefined, omit} from 'lodash';


Is this still a Controller after all of the modifications? If the answer is no, then let's not name it Cntlr. Controller has a defined behavior for us.

yes, I thought the file name should be changed. Thanks.

loitly

Thanks for the changes. Good to go.

robyww

Looks good.

I listed several small java issues.

UI:

the full hips list table should have the filters input turned on by default. I think this is very important since it is such a long table.
I don't think you need to show the source column since that is not really part of the data.

robyww · 2018-03-09T01:44:05Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hipslists/IrsaHiPSListSource.java

+    }
+
+
+    // this is temporary solution for getting HiPS from IRSA


If IRSA were to deploy some sort of hipslist service then that would be the final solution.

robyww · 2018-03-12T20:58:19Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hips/CDSHiPSListSource.java

+    private static final String HiPSCDSURL = AppProperties.getProperty("HiPS.cds.hostname",
+                                             "http://alasky.unistra.fr/MocServer/query?hips_service_url=*&get=record");
+    //set default timeout to 30seconds
+    int timeout  = new Integer( AppProperties.getProperty("HiPS.cds.timeoutLimit" , "30")).intValue();


this should be:
private static final int TIMEOUT

robyww · 2018-03-12T20:59:48Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hips/CDSHiPSListSource.java

+            return createHiPSList(dataTypes, source);
+        } catch (FailedRequestException | IOException | DataAccessException e) {
+            _log.info("get CDS HiPS failed");
+            e.printStackTrace();


we us Logger all logging so you should not use printStackTrace
so use this:

_log.warn(e,"get CDS HiPS failed");

if You think it needs to be info then call the log method directly to log an info with an exception.

robyww · 2018-03-12T21:02:06Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hips/CDSHiPSListSource.java

+
+        for (int i = 0; i < HiPSMasterList.HiPSDataType.length; i++) {
+            if (Arrays.asList(dataTypes).contains(HiPSMasterList.HiPSDataType[i])) {
+                dp = dp + "&dataproduct_type=" + HiPSMasterList.HiPSDataType[i];


You can do this:

dp+= "&dataproduct_type=" + HiPSMasterList.HiPSDataType[i];

robyww · 2018-03-12T21:13:25Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hips/IrsaHiPSListSource.java

+        }
+        catch (FailedRequestException | IOException e) {
+            _log.info("get Irsa HiPS failed");
+            e.printStackTrace();


logging: same as above

robyww · 2018-03-12T21:13:32Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hips/IrsaHiPSListSource.java

+            e.printStackTrace();
+            return null;
+        } catch (Exception e) {
+            e.printStackTrace();


logging: same as above

robyww · 2018-03-12T21:15:20Z

src/firefly/java/edu/caltech/ipac/firefly/server/visualize/hips/IrsaHiPSListSource.java

+            oneList.set(PARAMS.SOURCE.getKey(), source);
+            for (int i = 0; i < dataCols.length; i++) {
+                Object obj = row.getDataElement(dataCols[i].getKeyName());
+                String val = obj != null ? String.valueOf(obj) : null;


this can be obj.toString()

ejoliet · 2018-03-12T23:08:02Z

Is it only me or don't you feel that the checkbox 'popular hips maps' should be replace by the opposite and unchecked. What i mean, it would be more 'natural' to have the default popular list and a checkbox that enable an advanced and more complete list... just a comment about UX.

cwang2016 · 2018-03-12T23:21:07Z

If the checkbox is titled with the word like 'popular HiPS', that will give the user the hint that there is a selection for popular list. Anyhow, there is plan (soon) to make 'popular HiPS' option configurable by the app, and the default setting will be determined by the configuration.

… table containing available HiPS from various sources and add method to form table containing popular HiPS. create new file containing irsa hips info, irsa-hips-master-table.csv DM-13771 fix titles on 'choose image type' section, default unit for HiPS fov and units for distance tool, fix hips and footprint/marker issues related to hips rendering on some target like arp-220, footprint/marker rendering while the background image is scrolled or zoomed and footprint/marker rotate, move, and resize while hips fov is changed. fix function call for drawing circle. re-create footprint elements when the plot zoom factor is changed.

cwang2016 · 2018-03-20T19:08:14Z

Add more fixing per lssues listed in DM-13771
https://jira.lsstcorp.org/browse/DM-13771

fix the image select title list on image search panel
change the unit for distance tool to be all lower cases
fix drawing problem related to target like arp220
fix exception error on footprint/marker operation, like rendering, rotation, moving, resizing, image scrolling, etc.
update the drawing objects of footprint when the image zoom or hips fov changes.

robyww · 2018-03-20T19:15:47Z

You still need to enable the filters on the all HiPS (non-popular) table.

…rsion

cwang2016 requested review from robyww and loitly March 8, 2018 02:13

cwang2016 self-assigned this Mar 8, 2018

ejoliet reviewed Mar 8, 2018

View reviewed changes

loitly requested changes Mar 9, 2018

View reviewed changes

loitly approved these changes Mar 10, 2018

View reviewed changes

robyww requested changes Mar 12, 2018

View reviewed changes

robyww mentioned this pull request Mar 17, 2018

DM-13625: Do final work with coverage and clean up of HiPS UI issues #562

Merged

cwang2016 force-pushed the DM-13624-HiPSLOading branch from 41d0fe7 to d039353 Compare March 20, 2018 18:54

add missing parameter in function call

dc23c37

DM-13624: fixed bug: ZoomOptionsPopup not working for hips-fits conve…

5941eef

…rsion

robyww approved these changes Mar 20, 2018

View reviewed changes

robyww and others added 2 commits March 20, 2018 13:17

DM-13624: fixed bug with coverage not updating

6802140

DM-13624 enable filters on the all HiPS (non-popular) table

dae4e5e

cwang2016 merged commit 6941c91 into dev Mar 20, 2018

cwang2016 deleted the DM-13624-HiPSLOading branch March 20, 2018 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-13624 Create HiPS survey loading framework #558

DM-13624 Create HiPS survey loading framework #558

cwang2016 commented Mar 8, 2018

ejoliet Mar 8, 2018

robyww Mar 9, 2018

cwang2016 Mar 9, 2018

ejoliet Mar 9, 2018

loitly left a comment

loitly Mar 9, 2018

cwang2016 Mar 9, 2018

loitly Mar 9, 2018

cwang2016 Mar 9, 2018 •

edited

Loading

loitly Mar 9, 2018 •

edited

Loading

cwang2016 Mar 9, 2018

loitly Mar 9, 2018

loitly Mar 9, 2018

loitly Mar 9, 2018

loitly Mar 9, 2018

cwang2016 Mar 9, 2018

loitly Mar 9, 2018 •

edited

Loading

loitly Mar 9, 2018

cwang2016 Mar 9, 2018 •

edited

Loading

loitly Mar 9, 2018

cwang2016 Mar 9, 2018

loitly left a comment

robyww left a comment •

edited

Loading

robyww Mar 9, 2018

robyww Mar 12, 2018

robyww Mar 12, 2018

robyww Mar 12, 2018

robyww Mar 12, 2018

robyww Mar 12, 2018

robyww Mar 12, 2018

ejoliet commented Mar 12, 2018

cwang2016 commented Mar 12, 2018

cwang2016 commented Mar 20, 2018

robyww commented Mar 20, 2018

		@@ -0,0 +1,139 @@
		package edu.caltech.ipac.firefly.server.visualize.hipslists;

DM-13624 Create HiPS survey loading framework #558

DM-13624 Create HiPS survey loading framework #558

Conversation

cwang2016 commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loitly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwang2016 Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

loitly Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loitly Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwang2016 Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loitly left a comment

Choose a reason for hiding this comment

robyww left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejoliet commented Mar 12, 2018

cwang2016 commented Mar 12, 2018

cwang2016 commented Mar 20, 2018

robyww commented Mar 20, 2018

cwang2016 Mar 9, 2018 •

edited

Loading

loitly Mar 9, 2018 •

edited

Loading

loitly Mar 9, 2018 •

edited

Loading

cwang2016 Mar 9, 2018 •

edited

Loading

robyww left a comment •

edited

Loading