Skip to content

FIREFLY-1370: Implement Cascade-style plots for spectra#1499

Merged
jaladh-singhal merged 1 commit intodevfrom
FIREFLY-1370-cascade-plots
Feb 21, 2024
Merged

FIREFLY-1370: Implement Cascade-style plots for spectra#1499
jaladh-singhal merged 1 commit intodevfrom
FIREFLY-1370-cascade-plots

Conversation

@jaladh-singhal
Copy link
Copy Markdown
Member

@jaladh-singhal jaladh-singhal commented Feb 16, 2024

Fixes FIREFLY-1370

Adds cascading option to combine chart dialog which makes sure that plots don't overlap when combined.

Note: I changed two icon in chart panel's pinned mode (feel free to suggest better icons):

  • Show tables - because the arrow was pointing in wrong direction in Bi-view (I believe it was created with Tri-view in mind)
  • Combine chart - because it was looking more like swapping than combining

Testing

https://fireflydev.ipac.caltech.edu/firefly-1370-cascade-plots/firefly

  • Upload spectra files linked in the ticket (which have same wavelength range), pin charts and then combine -> Apply cascading with default padding -> check combined charts don't overlap.
  • Decrease padding to make cascading of spectra tighter.

This achieves what design ticket is asking for, but more testing is needed to find out the edge cases where it will fail.

  • It is known that it will fail if charts with different spectral axis units are combined - that's being tracked separately at FIREFLY-1414

@lrebull
Copy link
Copy Markdown
Contributor

lrebull commented Feb 16, 2024

IANAS (I Am Not A Spectroscopist) but it does seem to be working as specified.
I think the workflow for the merging of spectra is a little counter-intuitive, but that's a larger problem than just this implementation.
I am not sure about the icons, either, but I don't have any better suggestions.
The explanation of what you're doing for the padding and cascading is very clear and so users should be able to understand what it's doing and what it's asking for.

Copy link
Copy Markdown
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

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

Great job on this nice, clean solution. However, while I was able to test successfully using your sample tables, it didn't work with another set of tables I have. I'll send them to you. I am guessing it might be related to the double negative (--) appended to the formula.

Comment on lines +189 to +193
<Stack spacing={1}>
<FormControl orientation={'horizontal'} sx={{'.MuiSwitch-root': {margin: 0}}}>
<FormLabel>Apply Cascading: </FormLabel>
<Switch checked={doCascading} onChange={(e)=>setDoCascading(e.target.checked)}/>
</FormControl>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If a SwitchInputField isn't already present, we should add one. It'll remove the need to handle state and populate props outside of fields.

Copy link
Copy Markdown
Contributor

@robyww robyww Feb 20, 2024

Choose a reason for hiding this comment

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

You can use <CheckBoxInputField type='switch'/>

Copy link
Copy Markdown
Member Author

@jaladh-singhal jaladh-singhal Feb 21, 2024

Choose a reason for hiding this comment

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

I tried using type='switch' but it doesn't allow me to enable/disable switch through UI - probably needs some debugging. Also for a single switch (instead of list of switch options), I need label on left instead of right - I think writing a SwitchInputField might be worth it. Adding this to outstanding issues ticket so that we can merge this PR.

@jaladh-singhal jaladh-singhal force-pushed the FIREFLY-1370-cascade-plots branch from 6932720 to a0f8d61 Compare February 21, 2024 03:11
FIREFLY-1370: Add cascading algo to combine chart

FIREFLY-1370: Connect cascading algo to UI

FIREFLY-1370: Fix bug when min, max are negative
@jaladh-singhal jaladh-singhal force-pushed the FIREFLY-1370-cascade-plots branch from a0f8d61 to ba31351 Compare February 21, 2024 03:12
@jaladh-singhal jaladh-singhal merged commit f275b2d into dev Feb 21, 2024
@robyww robyww added the Charts Anything related to charts label Apr 5, 2024
@robyww robyww added this to the 2024.1 milestone Apr 5, 2024
@robyww robyww deleted the FIREFLY-1370-cascade-plots branch April 12, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Charts Anything related to charts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants