IRSA-629 and IRSA-630: add generic way to get downloader in time series tool #421

ejoliet · 2017-07-24T22:00:57Z

This pull request combined 2 tickets IRSA-629 and 630.

One (IRSA-629) causes other one (IRSA-630): in order to get a different download processor (other than the current fixed one for WISE), i have introduced a function to deal with it in the UI form the converter factory.
Then i have added a specific server-side processor to package the files to be downloaded with the help of Wei's ticket (IRSA-514). Firefly has IBE for PTF included now.

There is a current issue with the background monitor (@Loi will fix it soon), once is fixed, the test should be going to the Timeseries tool and upload a LC for PTF, then select any row of the table and download the file(s) to test.

…ad processor and PTF in particular

wmiipac · 2017-07-24T23:54:11Z

src/firefly/java/edu/caltech/ipac/firefly/server/query/ptf/PtfProcimsFileRetrieve.java

+ */
+
+
+@SearchProcessorImpl(id = "PtfProcimsFileRetrieve")


+@SearchProcessorImpl(id = "PtfProcimsFileRetrieve")
this processor id is the same with the one in ife but the getBaseURL parameter is different, in ife(gwt) the host, schemaGroup, schema and table are read in from the xml configuration. In firefly the params are set in the PTFPlotRequests and ptfDownloadOptPanel, "mission" is equivalent to schemaGroup. would it be confusion, when we build TST under irsaviewer? the two processor (one in firefly, the other in ife) with the same processor id. I would suggest using PtfTSProcimsFileRetrieve

Actually, none of those processor are used. IBE generic code in Firefly make use of edu.caltech.ipac.astro.ibe.datasource.PtfIbeDataSource#makeDataParam to determine the URL to download the file by single processor : IBE file retrieve processor.

I will actually remove them because there are useless. Thanks for catching that!

I've actually kept them because they are identical to the ife ones and i think they can be useful in the future. I just changed the id name to avoid confusion, but these classes are not for LC per se, they are for getting IBE PTF specifically.

ejoliet · 2017-07-25T02:11:56Z

BTW, if you want to run the newly test added to this branch, you will need to checkout IRSA-629-add-ptf-images branch under firefly_test_data repos.

wmiipac

Looks good, test runs well too. It is nice that the download processor can be called for specific mission now. Noted that the file download has pending issue with the background monitor, but not to be solved in this PR.
PR approved

loitly

Code looks fine and it works well.
I have a couple of comments, but they are not a big deal.
However, I've just push a commit that fixed the packaging regression bug. Please review and merge when ready.

loitly · 2017-07-25T20:32:33Z

src/firefly/java/edu/caltech/ipac/astro/ibe/datasource/PtfIbeDataSource.java

@@ -49,7 +52,7 @@
        public String getTable() { return table;}
    }

-    public PtfIbeDataSource() {}
+    public PtfIbeDataSource() { ptfResolver = new PtfIbeResolver();}


I think this assignment should be in initialize(). It awkward to see assignment in one constructor, but not in the others. After looking at it, the empty constructor and initialize() should be combined as a static creator. But, maybe next time.

there is only one constructor. is fine.

You are right. I will add it to setupDs() which is called by initialize anyway.

loitly · 2017-07-25T20:41:35Z

src/firefly/java/edu/caltech/ipac/firefly/server/query/lc/PtfFileGroupsProcessor.java

+//                    extName = null;
+//                }
+
+                fi = new FileInfo(url, extName, 100000);


I assume 100000 is an arbitrary number. I think if you don't know what the size of the file should be, just put 0.

should be '0' actually but it works seamlessly i think. Will change it.

loitly · 2017-07-25T20:44:27Z

src/firefly/java/edu/caltech/ipac/firefly/server/query/lc/PtfFileGroupsProcessor.java

+
+    private static String getBaseURL(ServerRequest sr) throws MalformedURLException {
+        // build service
+        return new PtfFileRetrieve().getBaseURL(sr);


Why is PtfFileRetrieve().getBaseURL(sr) not a static method.

left from previous code introduced in different access, can be static now. Will change.

loitly · 2017-07-25T20:53:05Z

src/firefly/java/edu/caltech/ipac/firefly/server/query/ptf/PtfIbeResolver.java

+        }
+
+
+        DataGroup dataObjects = DataGroupReader.readAnyFormat(tempFile);


Since we know it's an ipac table, no need to call this method and waste the overhead.
DataGroupReader.read(tempFile)

good point!

loitly · 2017-07-25T21:05:23Z

src/firefly/js/templates/lightcurve/LcResult.jsx

+
+    if(!convertData.downloadOptions){
+        downloaderOptPanel = defaultOptPanel;
+    }


This plus the assignment at top is a bit of a mind-twister.. Don't you mean this?
const downloaderOptPanel = convertData.downloadOptions || defaultOptPanel

Yep, will clean it up as i have to clean up other things too.

loitly · 2017-07-25T23:26:01Z

@ejoliet , FYI: I've just pushed another commit to fix 'Get Download Script' regression bug.

ejoliet · 2017-07-25T23:32:34Z

Great! i will look at it and let you know. THanks!

…

On 7/25/17 4:26 PM, Loi Ly wrote: @ejoliet <https://github.com/ejoliet> , FYI: I've just pushed another commit to fix 'Get Download Script' regression bug. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#421 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAvATJEAkNjegOYtnKRMoxWI63UUL43Bks5sRnmKgaJpZM4OhzBn>.

-- Emmanuel Joliet NASA/IPAC Infrared Science Archive MS 100-22, Caltech, Pasadena, CA 91125 Office: 168 Email: [email protected] Phone: (626) 395-1489

ejoliet requested review from wmiipac and loitly July 24, 2017 22:01

IRSA-629 and IRSA-630: add generic way to get mission specific downlo…

5a1133e

…ad processor and PTF in particular

wmiipac requested changes Jul 24, 2017

View reviewed changes

IRSA-629: rename ids to avoid confusion

dcc2f44

wmiipac approved these changes Jul 25, 2017

View reviewed changes

fix broken package download after recent changes

ac798cb

loitly approved these changes Jul 25, 2017

View reviewed changes

fix 'Get Download Script' regression bug

11f4baa

ejoliet added 2 commits July 25, 2017 17:57

IRSA-629: refactor and clean code based on reviewer feedback

f9431eb

IRSA-629: refactor and clean code based on reviewer feedback

eff30bd

ejoliet merged commit f979199 into dev Jul 26, 2017

ejoliet deleted the IRSA-629-add-ptf-image-downloader branch July 26, 2017 18:23

		}


		DataGroup dataObjects = DataGroupReader.readAnyFormat(tempFile);

		*/


		@SearchProcessorImpl(id = "PtfProcimsFileRetrieve")

IRSA-629 and IRSA-630: add generic way to get downloader in time series tool #421

IRSA-629 and IRSA-630: add generic way to get downloader in time series tool #421

Uh oh!

Conversation

ejoliet commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejoliet commented Jul 25, 2017

Uh oh!

wmiipac left a comment

Choose a reason for hiding this comment

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loitly commented Jul 25, 2017

Uh oh!

ejoliet commented Jul 25, 2017 via email

Uh oh!

Uh oh!