IRSA-540, IRSA-541:Add PTF case to Time Series Tool #409

lznakano · 2017-07-11T22:04:21Z

Two tickets were implemented together. A new PTF form is created and added. Corresponding actions in the form were implemented.

Add PTF option

wmiipac

both PTFMissionOptions and PTFplotRequsets serve as place holder for now, as they are converted from wise They will be rewritten for PTF image date retrieve when implementing IRSA-514.
Everything else looks fine. BTW PTF has green and red filters, no w1 and w2 band.

Correct the band names

loitly · 2017-07-12T00:52:48Z

src/firefly/js/templates/lightcurve/PTF/PTFMissionOptions.js

+                if ((colType.includes(col.type)) &&
+                    (!has(col, 'visibility') || get(col, 'visibility') !== 'hidden') &&
+                    (col.name.startsWith(val))) {
+                    prev.push(col.name);


Same comment here. This is copy/paste code that should be in util.

loitly · 2017-07-12T00:52:53Z

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

@@ -105,6 +105,12 @@ export class LcViewer extends PureComponent {
                    errorMsg = `The uploaded table is not valid. The ${getMissionName(converterId)} option requires frame_id, source_id, or both scan_id and frame_num.
                    Please select the "Other" upload option for tables that do not meet these requirements.`;
                }
+
+                if (converterId === 'ptf') {


LcViewer is generic code and should not have 'ptf' references.

Loi, this code has been introduced to deal with specific error and we said last time we will cleanup and refactor the code but never get to it. I think we have time to do it and i propose to Lijun to go ahead and spend time to at least do this part and cleanup as much as possible, specially because we will use again for ZTF.

@lznakano , my proposal is the following: make use of {{converters.wise.isTableUploadValid}} function from the LcConverterFactory.js.
I've introduced this function in the past but is not used in the common code, only in WiseMissionOption.js.

Among other cleanup that you might want to do and simply the code, I'd suggest at least to replace this piece of code:

if (converterId === 'ptf') { errorMsg = `The uploaded table is not valid. The ${getMissionName(converterId)} option requires pid. Please select the "Other" upload option for tables that do not meet these requirements.`; }

with:

errorMsg = converterData.isTableUploadValid(converterId)

or something similar.

Right now, the function isTableUploadValid returns a boolean value but you can return a function with {errorMsg:'yada yada', isValid:boolean} and use it in LcViewer.jsx to show the error message if validation fails.
Of course, you will need to add a specific function for PTF in PTFMissionOptions.js.

What do you think? @loitly @lznakano

error is a string and meant to be used as an error message. So, the code here can simply display the error message if exists. It should not even care about converterId. Mission specific message can be set in onNewRawTable. You can see wiseOnNewRawTable as an example.

loitly · 2017-07-12T00:52:56Z

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

+            (!has(col, 'visibility') || get(col, 'visibility') !== 'hidden')) {
+            prev.push(col.name);
+        }
+        return prev;


These sort of functions should be in TableUtil.js. It would also be easier to read if written as:

return get(rawTbl, ['tableData', 'columns']) .filter((col) => colType.includes(col.type) && get(col, 'visibility') !== 'hidden') .map((col) => col.name);

This method was in WiseMissionOption. I moved it here without making any changes. I will make the necessary changes and then put it in TableUtils.js.

loitly · 2017-07-12T00:55:14Z

src/firefly/js/templates/lightcurve/PTF/PTFMissionOptions.js

+ * @type {string[]}
+ */
+const xyColPattern = ['\\w*jd\\w*', 'w[1-4]mpro\\w*'];
+export function ptfOnNewRawTable(rawTable, missionEntries, generalEntries, converterData, layoutInfo) {


xyColPattern = ['\wjd\w', 'w[1-4]mpro\w*'];

This is wise's specific. I doubt it'll work for ptf.

Lijun, As I have commented before both PTFMissionOptions and PTFplotRequests will be rewritten for PTF data when implementing IRSA-514, don't need to make any changes as far as to dealing with PTF data specifics.

@wmiipac for ticket IRSA-514, you only need to care about PTFPlotRequests.js. Why do you care about PTFMissionOptions.js ? You shouldn't, right?
I agree that the request will be different of course but is fair to add code as part of the PR so you get a code to change only ;-)

loitly · 2017-07-12T01:00:02Z

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

+/**
+ * Returns only numerical column names form raw lc table
+ * @param {Object} rawTbl
+ * @returns {TableColumn[]} - array of table columns objects


This is wrong. It's returning string[] - array of column names, not TableColumn[].

lznakano · 2017-07-12T17:25:39Z

Refactor and rearrange the codes will be done after real PTF specific parameters are in place.

ejoliet

Thank you for the PR! Couple of comments:
As Loi suggested, we shouldn't have any code specific in the common code. Only in the mission options panel. Please, remove the PTF code from common part otherwise it will become unmaintanible.
Also, take out or commented out the WISE code in the image specific function, is probably going to be reused anyway but not with same pattern.

That would be good to have the cleanup/refactor as much as possible before merging. If you see the opportunity to identify repeated code, please move it in util class so it cna be reused most probably by ZTF in the (near future).

Overall is good progress!

I also suggest as part of this ticket to add the PTF option in IRSAViewer (IFE) part.

Thank you!

ejoliet · 2017-07-12T18:54:30Z

Also, the band label should display a lower-case 'g' not 'G'. 'R' is fine.
Thanks!

ejoliet · 2017-07-12T20:08:59Z

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

@@ -96,7 +96,7 @@ export class LcViewer extends PureComponent {
        const LcPeriodInstance=  get(getAppOptions(), 'charts.chartEngine')==='plotly' ? LcPeriodPlotly : LcPeriod;

        var mainView = (err,converterId) => {
-            if (!isEmpty(error) && converterId) {
+            if (error!='undefined' && !isEmpty(error) && converterId) {



i thought isEmpty functionn from lodash covered the case error!='undefined', is not? WOW, I'm surprised! i've used elsewhere wrongly then. @loitly @robyww did you know that?

isEmpty(undefined) will return true. but here, it's checking for a string 'undefined'.

ejoliet · 2017-07-12T20:14:56Z

src/firefly/js/templates/lightcurve/LcConverterFactory.js

 import {DefaultSettingBox, defaultOnNewRawTable, defaultOnFieldUpdate, defaultRawTableRequest} from './generic/DefaultMissionOptions.js';
 import {BasicSettingBox, basicOnNewRawTable, basicOnFieldUpdate, basicRawTableRequest, imagesShouldBeDisplayed} from './basic/BasicMissionOptions.js';
+import {WiseSettingBox, wiseOnNewRawTable, wiseOnFieldUpdate, wiseRawTableRequest, isBasicTableUploadValid} from './wise/WiseMissionOptions.js';
+import {PTFSettingBox, ptfOnNewRawTable, ptfOnFieldUpdate, ptfRawTableRequest} from './PTF/PTFMissionOptions.js';


Please, change to lower-case 'ptf' directory. it make more sense and is cleaner. Thanks.

Refactor the codes in this package

lznakano · 2017-07-17T19:01:38Z

I have done lot of refactoring. There are so many duplications. I combined some methods to one which can be used by all missions and also moved some common methods to the LcUtil.jsx. After PTF images are added, it is possible to do further refactoring.

I have pushed ife branch with the same name "IRSA-540-IRSA-541-PTF". Please check out firefly and ife branches and have a look. Thanks!

ejoliet · 2017-07-17T22:17:50Z

I've built it and didn't see any problem. PTF case is in IRSAViewer.
Code refactored looks cleaner!
You might want to ask quick review by @loitly before merging though.
Once merge, let @wmiipac know to add/fix the IBE function to get the images.
Thank you!

lznakano · 2017-07-17T23:06:39Z

There are some duplications in wise and ptf options. I removed them last Friday but added back today as place holders. RawTableRequest and FiledUpdate are such functions. I would wait until the PFT image loaded to see if these two functions in two mission need to be different or not.

…cUtils.js

loitly

Code looks fine. I cleanup TableUtils.js and push my changes. I did not do testing because I do not have a ptf file readily available. But, I assumed @ejoliet have. Good to go.

loitly · 2017-07-18T01:17:04Z

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

+                [LC.META_FLUX_BAND]: Object.assign(getTypeData(LC.META_FLUX_BAND, '',        '' +
+                    'Select WISE band for images to be displayed',        'Image display:', 70)),
+                [LC.META_ERR_CNAME]: Object.assign(getTypeData(LC.META_ERR_CNAME, '',
+                    'value error column name',       'Error Column:', labelWidth))


FYI: I removed a few non-ascii characters from here. However, the code could use some cleaning up. Object.assign() is used with only 1 parameter, why? Also, why is there a '' + 'Select...'

clean up none used codes

Add comments

IRSA-540, IRSA-541:

4c10e3c

Add PTF option

lznakano requested review from ejoliet and wmiipac July 11, 2017 22:04

wmiipac reviewed Jul 11, 2017

View reviewed changes

IRSA-540-IRSA-541-PTF:

908aef5

Correct the band names

loitly reviewed Jul 12, 2017

View reviewed changes

ejoliet requested changes Jul 12, 2017

View reviewed changes

ejoliet reviewed Jul 12, 2017

View reviewed changes

Lijun Zhang added 2 commits July 14, 2017 17:19

Refactor LightCurve package

37c9a8a

IRSA-540, IRSA-541

a60fb50

Refactor the codes in this package

ejoliet approved these changes Jul 17, 2017

View reviewed changes

IRSA-540: cleanup TableUtil.js and remove non ascii characters from L…

5624418

…cUtils.js

loitly approved these changes Jul 18, 2017

View reviewed changes

loitly and others added 3 commits July 18, 2017 10:06

minor tweak to the functions added to TableUtils.js

3407035

IRSA-540-IRSA-541-PTF:

a7d2820

clean up none used codes

RSA-540-IRSA-541-PTF:

ff453d1

Add comments

lznakano merged commit 53efe27 into dev Jul 18, 2017

lznakano deleted the IRSA-540-IRSA-541-PTF branch July 18, 2017 17:58

IRSA-540, IRSA-541:Add PTF case to Time Series Tool #409

IRSA-540, IRSA-541:Add PTF case to Time Series Tool #409

Uh oh!

Conversation

lznakano commented Jul 11, 2017

Uh oh!

wmiipac 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!

ejoliet Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lznakano commented Jul 12, 2017

Uh oh!

ejoliet left a comment

Choose a reason for hiding this comment

Uh oh!

ejoliet commented Jul 12, 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!

lznakano commented Jul 17, 2017

Uh oh!

ejoliet commented Jul 17, 2017

Uh oh!

lznakano commented Jul 17, 2017

Uh oh!

loitly left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ejoliet Jul 12, 2017 •

edited

Loading

loitly left a comment •

edited

Loading