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

HyperSpectra: change the default integration method #754

Merged
merged 1 commit into from
Oct 5, 2024

Conversation

markotoplak
Copy link
Collaborator

This one also works for "short" spectra, like images with 1 column.

The problem is that if you open an image that has only one "spectral" column, you see nothing useful because the default integral is not working (=returns all the same values). I propose changing it.

I did not want to do any smart detection because then whatever would be detected would also be saved as a default setting. We could perhaps do detection if these settings were "schema only".

This one also works for "short" spectra, like images with 1 column.
@borondics
Copy link
Member

I was thinking about this a bit. The previous default, when we did the whole integral should also work for one column, no? If that is OK or we can make it work, we can keep the old default and later come up with a nicer clever solution that handles single column and multicolumn data as well.

@markotoplak
Copy link
Collaborator Author

Integral of a function that defines one non-zero value on the real axis is zero, so we can not make that option work if we do not want to do some very artificial exceptions.

I would change the default, because the old one (integral on a the whole data width) rarely made much sense.

@borondics
Copy link
Member

What do we select then for hyperspectral datasets? Having the first value in the spectral domain makes also little sense... At least with the full integral we immediately saw a nice signal-to-noise image that actually says something about the measurement as a whole.

Can we make a hybrid version with logic (one col vs. multiple cols)? This can also come later, doesn't need to be now. Actually, many of the plotting variants don't work for one column so we should make them grey when there is only one column or remove them from the list?

Ahh... life is never simple, is it? :)

@markotoplak
Copy link
Collaborator Author

It is the same issue as with the color selector. If I do something smart, I break the settings unless I also modify them to schema_only or something.

But here I think that this a really nice simple default.

@markotoplak
Copy link
Collaborator Author

markotoplak commented Oct 4, 2024

Oh, for hyperspectral data you then get the middle wavelength.

@markotoplak markotoplak marked this pull request as draft October 4, 2024 13:41
@stuart-cls
Copy link
Member

Oh, for hyperspectral data you then get the middle wavelength.

This is what Agilent's software does. It can be misleading about the hyperspectral cube as @borondics says. But it is fast (no integral calculation) which is nice for large datasets, I usually leave mine set to "closest value" (as a setting) when working with large datasets.

@borondics
Copy link
Member

Well, I am totally for speed in general! But then, let's have some fun and pick the wavelength randomly. :D What do you guys think?

@markotoplak
Copy link
Collaborator Author

No, @borondics, we are not doing it random. :)

So, do I put this into the release with the scaling factors?

@borondics
Copy link
Member

Sure, let's have the middle point (which is also random, but the random number generator is the user, so I'm happy).

@markotoplak markotoplak changed the title HyperSpectra: change default integration method HyperSpectra: change the default integration method Oct 5, 2024
@markotoplak markotoplak marked this pull request as ready for review October 5, 2024 08:55
@markotoplak markotoplak merged commit 0459bf7 into Quasars:master Oct 5, 2024
10 of 14 checks passed
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