Skip to content

FIREFLY-428: Spectral Viewer Phase 1 RC#1066

Closed
loitly wants to merge 1 commit intodevfrom
FIREFLY-428_chart_auto_split
Closed

FIREFLY-428: Spectral Viewer Phase 1 RC#1066
loitly wants to merge 1 commit intodevfrom
FIREFLY-428_chart_auto_split

Conversation

@loitly
Copy link
Copy Markdown
Contributor

@loitly loitly commented Mar 17, 2021 •
edited
Loading

Test: https://fireflydev.ipac.caltech.edu/firefly-428-chart-auto-split/firefly/

I am listing all of the tickets I've looked at and worked on. Some, only partially, due to time constraint or requiring additional requirements.

https://jira.ipac.caltech.edu/browse/FIREFLY-691 (phase 1 requirement)

  • Different order will be assigned a different color, but with the same symbol. Both, can be changed from the settings dialog.
  • TODO
    • How to identify a spectra vs a SED? ipac:Spectrum.SED is this a table utype?
    • The hover text for a point should include order information if it is present.

https://jira.ipac.caltech.edu/browse/FIREFLY-686 (create new spetral viewer component)

  • Done

https://jira.ipac.caltech.edu/browse/FIREFLY-428 (auto-split spectra with 'order' info)

  • Mostly done, except for some advance UI concepts I do not understand

https://jira.ipac.caltech.edu/browse/FIREFLY-269 (only unit conversion)

  • Used this ticket for initial unit conversion work. Other items on the ticket has not been worked on.

https://jira.ipac.caltech.edu/browse/FIREFLY-733 (release multitrace UI to ops)

  • Done. However, due to this change, there are cases when there's only 1 field under 'Trace Options' (Heatmap).

https://jira.ipac.caltech.edu/browse/FIREFLY-745 (modifications to Spectral Viewer Prototype)

The goal of this Phase 1 RC is to tie up loose the ends so it can be released. Please help thoroughly test this version for bugs. I will fix as much as I can. Bigger items will be addressed in phase 2.

Additional changes not specified in the above tickets:

  • Updated plotly from v1.49.4 to v1.58.4. This is needed to add Legend's title to chart.
  • Allow active trace switching by clicking on data points on the chart
  • Minor UI changes
    • Remove borders to make dialog less cluttered
    • Add max height to dialog with auto scrollbar

@loitly loitly requested review from ejoliet and robyww March 17, 2021 19:51
@loitly loitly self-assigned this Mar 17, 2021
@ejoliet
Copy link
Copy Markdown
Contributor

ejoliet commented Mar 17, 2021

FIREFLY-686 has one sample including specOrder example. See file name: SPITZER_S0_25343744_0001_3_E7173899_tune.votable

@loitly loitly force-pushed the FIREFLY-428_chart_auto_split branch from 5741a3c to 0e96f9d Compare March 17, 2021 22:57
ejoliet
ejoliet approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ejoliet ejoliet left a comment

Choose a reason for hiding this comment

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

Great!
I've added the sample file to the FIREFLY-428 ticket with the added reference field to the wavelength axis group so anyone can test it. It works like a charm!

robyww
robyww approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

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

Looks good.

Suggestions:

  • You might want to try using async/await in FireflyGenericData.js. I find it improves readability.
  • You might want to go ahead a convert ScatterOptions to be a function component. I am trying to do that when I have to modify any component. I would like to see the rest of SimpleComponent subclasses get upgraded.

Comment thread src/firefly/js/charts/dataTypes/FireflyGenericData.js Outdated
dispatchError(chartId, traceNum, tableModel.error);
return;
}
Promise.all([sreq, ...tblReqs].map((r) => doFetchTable(r))).then((tablemodels) => {
Copy link
Copy Markdown
Contributor

@robyww robyww Mar 18, 2021

Choose a reason for hiding this comment

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

have you thought about trying await for this?

const tableModels= await Promise.all([sreq, ...tblReqs].map((r) => doFetchTable(r)));

@loitly loitly force-pushed the FIREFLY-428_chart_auto_split branch from 0e96f9d to 9fd677c Compare April 13, 2021 19:38
@loitly
FIREFLY-428: Provide for auto-split on a category column for creation…
95ef48c 
… of multiple traces

- introduce Firefly specific utype: spec:Spectrum.Data.FluxAxis.Order
- introduce Firefly specific utype: spec:Spectrum.Data.FluxAxis.Accuracy.[StatHigh,StatLow] for upper/lower limit.  This may apply to SpectralAxis as well.
- when FluxAxis.Order is specified, must use presets without any additional traces
- add 'Use spectrum presets' option to setting dialog
- allow active trace switching by selecting data points on the chart
- allow active trace switching while in the settings dialog
- FIREFLY-734: remove SingleTraceUI option, making multitrace the only choice
- Legend and Trace Name will only appears when there's more than 1 trace
- UI revamped
- refactoring
- update plotly from v1.49.4 to v1.58.v
@loitly loitly force-pushed the FIREFLY-428_chart_auto_split branch from 9fd677c to 95ef48c Compare April 13, 2021 23:48
@loitly loitly closed this Apr 13, 2021
@loitly loitly reopened this Apr 13, 2021
@loitly loitly changed the title FIREFLY-428: Provide for auto-split on table with SpectralOrder FIREFLY-428: Spectral Viewer Phase 1 RC Apr 14, 2021
robyww
robyww approved these changes Apr 14, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@robyww robyww left a comment

Choose a reason for hiding this comment

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

Code

I is really too much code to review, but what I looked at looks good. I did not notice you changed some SimpleComponent classes to functional. That's great!

UI

  • I look really good to me. stuff seems to work as advertised, though I did not click on everything.
  • One comment: I don't think our non-editable fields should look like the editable. InputFieldView should probably render a different look when readonly is set. The only way a user knows he can't edit them is by clicking and wonder what is wrong. To the user it probably feels like a bug.

@vandesai1
Copy link
Copy Markdown

vandesai1 commented Apr 15, 2021 •
edited by robyww
Loading

  • attempting to change x-axis units does not work
  • attempting to change y-axis units does not work
  • inconsistent point value tooltip: when order 2 is selected, the tooltip will show wavelength, flux_density with errorbar for either order. When order 3 is selected, it will not show the uncertainty for a point in order 2, but will show the uncertainty for a point in order 3.
  • Firefly doesn't recognize SPITZER_S3_3757824_0013_12_E7212632_tune.votable or F0291_EX_SPE_04012012_EXEELONEXEECHL_CMB_0040-0043.votable as having orders, no order testing possible.
  • I think we want "Plot Style"-->"Trace Style"
  • The "choose trace" menu is really small so my chosen trace name doesn't fit in it even though there is plenty of space.

@loitly loitly closed this Apr 19, 2021
@robyww robyww deleted the FIREFLY-428_chart_auto_split branch April 21, 2021 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ejoliet ejoliet ejoliet approved these changes

@robyww robyww robyww approved these changes

Assignees

@loitly loitly

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@loitly @ejoliet @vandesai1 @robyww