Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: generate and display stocsy #3055

Merged
merged 34 commits into from
May 27, 2024
Merged

feat: generate and display stocsy #3055

merged 34 commits into from
May 27, 2024

Conversation

hamed-musallam
Copy link
Member

No description provided.

@hamed-musallam hamed-musallam linked an issue May 21, 2024 that may be closed by this pull request
Copy link

cloudflare-workers-and-pages bot commented May 21, 2024

Deploying nmrium with  Cloudflare Pages  Cloudflare Pages

Latest commit: aad389f
Status: ✅  Deploy successful!
Preview URL: https://dc949a81.nmrium.pages.dev
Branch Preview URL: https://display-boxplot-stocsy.nmrium.pages.dev

View logs

@targos
Copy link
Member

targos commented May 21, 2024

cf build error is:

15:18:52.548 | x Build failed in 6.21s
-- | --
15:18:52.550 | error during build:
15:18:52.550 | RollupError: Could not resolve "../../context/ExportPrepareContext" from "src/component/1d/matrix/Stocsy.tsx"
15:18:52.550 | file: /opt/buildhome/repo/src/component/1d/matrix/Stocsy.tsx
15:18:52.550 | at getRollupError (file:///opt/buildhome/repo/node_modules/rollup/dist/es/shared/parseAst.js:376:41)
15:18:52.551 | at error (file:///opt/buildhome/repo/node_modules/rollup/dist/es/shared/parseAst.js:372:42)
15:18:52.551 | at ModuleLoader.handleInvalidResolvedId (file:///opt/buildhome/repo/node_modules/rollup/dist/es/shared/node-entry.js:18891:24)
15:18:52.551 | at file:///opt/buildhome/repo/node_modules/rollup/dist/es/shared/node-entry.js:18851:26
15:18:52.629 | Failed: Error while executing user command. Exited with error code: 1

@hamed-musallam hamed-musallam force-pushed the display-boxplot-stocsy branch from 9038734 to cf213c0 Compare May 21, 2024 13:30
@hamed-musallam hamed-musallam force-pushed the display-boxplot-stocsy branch 3 times, most recently from 5671722 to 6864ca3 Compare May 21, 2024 16:43
@lpatiny
Copy link
Member

lpatiny commented May 22, 2024

  1. Please use SVG and allow vertical scale. We will improve with grouped path by color very soon like Norman did in the visualizer and JsGraph
  2. For now please use the first row as Y axis and paint the color with the returned value from stocsy
  3. deal with the index on which the user clicked

@hamed-musallam hamed-musallam force-pushed the display-boxplot-stocsy branch 4 times, most recently from 4ad07c1 to 028080c Compare May 24, 2024 10:34
@hamed-musallam hamed-musallam marked this pull request as ready for review May 24, 2024 10:40
@hamed-musallam hamed-musallam force-pushed the display-boxplot-stocsy branch 2 times, most recently from 0af5f93 to ccefba0 Compare May 24, 2024 11:06
@lpatiny
Copy link
Member

lpatiny commented May 24, 2024

If I only display the STOCSY

image

And I just moved the scroll wheel (without ALT), nothing happened (as expected), but nevertheless, the STOCSY was redrawn.

image

@lpatiny
Copy link
Member

lpatiny commented May 24, 2024

@hamed-musallam For the boxPlot and the calculation of the 3 paths please use xyReduce. Becuase we have only color it can be used in this case.

@lpatiny
Copy link
Member

lpatiny commented May 24, 2024

It seems that currently the SVG is painted outside the visible div as well. This can be observed when you donwload the SVG and open it in inkscape

image

And this could explain it is also relatively slow when zoomed in.

@hamed-musallam hamed-musallam force-pushed the display-boxplot-stocsy branch 2 times, most recently from 2e4f537 to d89ee5e Compare May 24, 2024 21:10
@lpatiny lpatiny force-pushed the display-boxplot-stocsy branch 2 times, most recently from 25204f1 to 7520e7f Compare May 25, 2024 08:07
@lpatiny
Copy link
Member

lpatiny commented May 25, 2024

Still small improvements that could be done:

  1. Hiding spectra yield to an error in the console

2024-05-25 10 08 39

  1. Confusion with 'Apply processing' and 'Update processing'

The first time we open Matrix generation the button should be labeled 'Apply processing' and after we could have 2 buttons:

  • Update processing: if any of the parameters change and there is a reason to update the processing
  • Remove processing: this will remove the filter 'Signal processing' from all the spectra. This button should be active if there is at least one spectrum with 'Signal processing'
  1. Show stocsy / boxplot

The 2 toggle button should only be available if the matrix has been generated and therefore that 'Apply processing' was clicked. We should not have a matrix before the 'Apply processing' because if we superiimpose spectra from different origins we will be in trouble.

So those button should rely on the presence of the matrix.

  1. Vertical scroll generates a lot of internal work by react (I guess)

Not sure how this can be improved but vertical scroll wheel even if there is no redraw takes around 100mS

2024-05-25 10 20 22

  1. Issues when selecting exclusion zones and vertical scaling

Once we have selected the select zone option there is no way to vertical scale the STOCSY anymore

2024-05-25 10 46 53

  1. Save / reload spectra

When we save a STOCSY with all the spectra hidden we can not reload it

XTC 1.nmrium.zip

@hamed-musallam
Copy link
Member Author

hamed-musallam commented May 27, 2024

  1. Hiding spectra yield to an error in the console

The warning you see is from Blueprint.js, indicating that a tooltip is active but its content is either empty, null, or undefined.

I will implement a temporary fix for this issue in NMRium. However, the underlying problem should be addressed in the react-science button component by checking the tooltip content and disabling it if it is empty.

i created a PR in react-science the zakodium-oss/react-science#730

hamed-musallam and others added 24 commits May 27, 2024 15:22
@hamed-musallam hamed-musallam force-pushed the display-boxplot-stocsy branch from c5470cd to 3bb7049 Compare May 27, 2024 13:22
@hamed-musallam hamed-musallam merged commit 83ff531 into main May 27, 2024
11 checks passed
@hamed-musallam hamed-musallam deleted the display-boxplot-stocsy branch May 27, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare code to allow STOCSY in multispectra
3 participants