Skip to content

feat: carbon solvent labelling - #3693

Merged
lpatiny merged 11 commits into
mainfrom
set-sensitivity-default-value
Oct 13, 2025
Merged

feat: carbon solvent labelling#3693
lpatiny merged 11 commits into
mainfrom
set-sensitivity-default-value

Conversation

@jobo322

@jobo322 jobo322 commented Sep 11, 2025

Copy link
Copy Markdown
Member

No description provided.

@jobo322 jobo322 changed the title feat: example of auto ranges options to works with carbon spectrum an… feat: example of auto ranges options to works with carbon solvent labelling Sep 11, 2025
Comment thread src/component/reducer/actions/RangesActions.ts Outdated

@lpatiny lpatiny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I try on ethyl benzene, the auto range picking yields to an error

image

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Oct 3, 2025

Copy link
Copy Markdown

Deploying nmrium with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4063806
Status: ✅  Deploy successful!
Preview URL: https://506621b5.nmrium.pages.dev
Branch Preview URL: https://set-sensitivity-default-valu.nmrium.pages.dev

View logs

@jobo322
jobo322 marked this pull request as ready for review October 7, 2025 14:37
@jobo322 jobo322 changed the title feat: example of auto ranges options to works with carbon solvent labelling feat: carbon solvent labelling Oct 7, 2025
@jobo322

jobo322 commented Oct 8, 2025

Copy link
Copy Markdown
Member Author

currently is it possible to have this result:

image

due the peak solvent has this shape

image

With peakpicking sensitivity options in 90, it does not happened but my question is if the carbon solvent labeling should iterate until a pattern of solvent is not present in the peak list, I mean, should the carbon solvent labeling function to find two solvent ranges and merge them if those are overlapped?

@jobo322

jobo322 commented Oct 8, 2025

Copy link
Copy Markdown
Member Author

also, another question related with the peak picking sensitivity option, should have a default value of 100? I mean without any smoothness at the moment to make the peakpicking?

@lpatiny lpatiny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain us what is this value ? Also please take the opportunity to type the options with explanation and default values.

@jobo322

jobo322 commented Oct 9, 2025

Copy link
Copy Markdown
Member Author

Could you explain us what is this value ? Also please take the opportunity to type the options with explanation and default values.

Yes, the peaks picking option sensitivity is documented in the interface of xyAutoPeaksPicking options

@jobo322

jobo322 commented Oct 9, 2025

Copy link
Copy Markdown
Member Author

currently is it possible to have this result:

image due the peak solvent has this shape image With peakpicking sensitivity options in 90, it does not happened but my question is if the carbon solvent labeling should iterate until a pattern of solvent is not present in the peak list, I mean, should the carbon solvent labeling function to find two solvent ranges and merge them if those are overlapped?

@lpatiny what about this possible refactoring of the carbon solvent labelling function

Comment thread src/data/data1d/Spectrum1D/peaks/autoPeakPicking.ts

@lpatiny lpatiny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jobo322 Please rebase the code and fix the test cases.

@jobo322
jobo322 requested a review from targos October 10, 2025 15:40
@targos

targos commented Oct 10, 2025

Copy link
Copy Markdown
Member

I am not really qualified to review these changes.

@lpatiny
lpatiny merged commit 720722b into main Oct 13, 2025
12 checks passed
@lpatiny
lpatiny deleted the set-sensitivity-default-value branch October 13, 2025 18:14
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.

3 participants