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

change the contour plot behavior #3050

Merged

Conversation

jobo322
Copy link
Member

@jobo322 jobo322 commented May 17, 2024

  • do not calculate noise every time the wheel is actioned.
  • do not include the max value of the data in the contour plot, it will contain a slice of the spectrum.
  • new contour levels by default are [0 - 100], the 0 level is the noise level.
  • new option numberOfZoomLevels is the number of steps, the levels will be sampled in an exponential scale (1.25**zoom) where zoom is an integer value between [0-`numberOfZoomLevels].

@jobo322 jobo322 linked an issue May 17, 2024 that may be closed by this pull request
Copy link

cloudflare-pages bot commented May 17, 2024

Deploying nmrium with  Cloudflare Pages  Cloudflare Pages

Latest commit: b5963a6
Status: ✅  Deploy successful!
Preview URL: https://dcf332b6.nmrium.pages.dev
Branch Preview URL: https://1337-contour-plot-does-not-a.nmrium.pages.dev

View logs

@jobo322 jobo322 changed the title fix: do not calculate noise every time the wheel is actioned change the contour plot behavior May 23, 2024
Copy link
Member

@lpatiny lpatiny left a comment

Choose a reason for hiding this comment

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

The functionalities looks better but the test are not passing. Please have a look why.

@hamed-musallam Please review the code as well.

@hamed-musallam hamed-musallam force-pushed the 1337-contour-plot-does-not-allow-to-define-the-number-of-contours branch from d65be31 to b213644 Compare June 10, 2024 14:25
@jobo322 jobo322 force-pushed the 1337-contour-plot-does-not-allow-to-define-the-number-of-contours branch from b213644 to d65be31 Compare June 10, 2024 15:20
@lpatiny
Copy link
Member

lpatiny commented Jun 15, 2024

@hamed-musallam Could you change the behaviour so that when we move the sliders the spectrum is updated on the fly (and not on mouse release like currently) ?

A small debounce will certainly be required.

2024-06-15 12 56 27

@targos
Copy link
Member

targos commented Jun 15, 2024

Do we want these new very large files in the repo?

@lpatiny
Copy link
Member

lpatiny commented Jun 15, 2024

Do we want these new very large files in the repo?

@jobo322 Please use 512 x 512, no need to have 1024 on 1024 I think.
Also don't do it 'pretty'. Simply JSON.stringify without the '2'

The files should then be around 1.3 Mb. Just by removing the \n + spaces I went from 22Mb to 5.5 Mb.

Michael and me are also wondering if we can directly load a NMRium file. Because in this case we would also have the zip compression.

@jobo322 jobo322 force-pushed the 1337-contour-plot-does-not-allow-to-define-the-number-of-contours branch from f910402 to 08fad9a Compare June 17, 2024 14:05
@lpatiny
Copy link
Member

lpatiny commented Jun 17, 2024

Functionality looks ok to me but for the manual change of slider that should update the contour plot after debouncing (before release of the cursor).
@hamed-musallam Could you check this ?

@hamed-musallam hamed-musallam force-pushed the 1337-contour-plot-does-not-allow-to-define-the-number-of-contours branch from f63a7b6 to efd5d2f Compare June 24, 2024 10:33
@hamed-musallam
Copy link
Member

What is left is to refactor the spectrum object and move the contours Options object to view which can be done in another PR

{
contourOptions:{
          positive: {
            contourLevels: [15, 100],
            numberOfLayers: 10,
            numberOfZoomLevels: 10,
          },
          negative: {
            contourLevels: [15, 100],
            numberOfLayers: 10,
            numberOfZoomLevels: 10,
          }
}

@hamed-musallam
Copy link
Member

The functionalities looks better but the test are not passing. Please have a look why.

@hamed-musallam Please review the code as well.

it is fixed

@targos targos self-assigned this Jun 24, 2024
Copy link
Member

@lpatiny lpatiny left a comment

Choose a reason for hiding this comment

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

Functionality looks ok to me

src/data/data2d/Spectrum2D/contours.ts Outdated Show resolved Hide resolved
src/data/data2d/Spectrum2D/contours.ts Outdated Show resolved Hide resolved
src/data/data2d/Spectrum2D/contours.ts Outdated Show resolved Hide resolved
@targos
Copy link
Member

targos commented Jun 25, 2024

Can be merged after addressing my comments.

@hamed-musallam
Copy link
Member

Can we merge this PR, or are there still comments?

@jobo322 jobo322 merged commit 1b62142 into main Jun 28, 2024
11 checks passed
@jobo322 jobo322 deleted the 1337-contour-plot-does-not-allow-to-define-the-number-of-contours branch June 28, 2024 11:56
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.

Contour plot does not allow to define the number of contours
4 participants