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

allow background parameter of Probe to be negative #218

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

bmaranville
Copy link
Member

When fitting data that has been "oversubtracted", sometimes the apparent background level is negative.

Setting a hard lower "physical" limit of 0.0 for the background parameter makes it hard to fit this type of dataset, without manually adjusting the limits after the Probe object is created

Currently, you would have to do e.g. this, to fit a negative apparent background (after initializing Probe object):

probe.background.limits = (-float("inf"), float("inf"))

@acaruana2009
Copy link
Contributor

I think this is fine, I have discussed it with some instrument scientists in my group and they are fine with it as a pragmatic solution to the problem.

@alexander-grutter
Copy link

One issue I've been wrestling with is how to display the data. Obviously negative reflectivity is nonsense and also it's hard to put on a log scale. What do we think about shifting the data by the fitted background offset? Feels iffy but all other solutions feel worse.

@bmaranville
Copy link
Member Author

bmaranville commented Nov 6, 2024

I don't think it's completely wrong to shift the data by the background, but then how do you visually keep track of the background parameter being fit? I guess we can add another checkbox "apply background shift"

Why do we even offer it as a fittable parameter? Because reduction is not perfect...

@alexander-grutter
Copy link

That's actually a feature I've wanted before. Again, it feels iffy, but I can see the value if applied judiciously.

@hoogerheide
Copy link
Contributor

I have used a symmetric log plot for this in the past: https://journals.iucr.org/j/issues/2022/01/00/ge5108/

I really like these plots, mainly because you can see all of the negative data and, importantly, whether there is any structure in the negative data. The proposal to just shift the background will still hide all of the negative data, though I do think it's a good start.

The downside is that they are somewhat difficult to generate. Matplotlib has symlog scaling but I don't know how to do this in plotly.

@hoogerheide
Copy link
Contributor

negative reflectivity is nonsense

Negative reflectivity is nonsense for the model, but negative measured reflectivity is not, because you're looking at the difference between two statistical quantities which can in fact be less than zero.

@bmaranville
Copy link
Member Author

Symlog has been requested from plotly for the past 8+ years... no movement to implement as far as I can tell.

@acaruana2009
Copy link
Contributor

I have used a symmetric log plot for this in the past: https://journals.iucr.org/j/issues/2022/01/00/ge5108/

I really like these plots, mainly because you can see all of the negative data and, importantly, whether there is any structure in the negative data. The proposal to just shift the background will still hide all of the negative data, though I do think it's a good start.

The downside is that they are somewhat difficult to generate. Matplotlib has symlog scaling but I don't know how to do this in plotly.

That is super nice! For spin flip data this would be very useful indeed! I have never even thought about plotting our data up in a symmetric log plot.

@alexander-grutter
Copy link

negative reflectivity is nonsense

Negative reflectivity is nonsense for the model, but negative measured reflectivity is not, because you're looking at the difference between two statistical quantities which can in fact be less than zero.

Yes - sorry this is a key point. What I should have said is that one does not expect to measure an excessively statistically significant net reflectivity. That's also not a great way to phrase it, but I think you know what I mean. Near the background, one does of course expect individual points to be below zero, and that's important information we shouldn't discard.

@bmaranville
Copy link
Member Author

Ok, I looked at plotly in depth and adding symlog to that is a major project, not something we can do.

Should we merge this and make a new issue for how to modify the display, or does everyone agree on what should be shown for the reflectivity wrt. background?

@hoogerheide
Copy link
Contributor

Should we merge this and make a new issue for how to modify the display, or does everyone agree on what should be shown for the reflectivity wrt. background?

I think this is the right way to proceed.

@acaruana2009
Copy link
Contributor

I am happy for this to be merged.

@andyfaff
Copy link
Contributor

andyfaff commented Nov 7, 2024

A comment from the sidelines. I frequently model XRR data on a logR transformed scale, because I find that the uncertainties that are often assigned during reduction aren't as good as they could be. A logR transform seems to work quite well. In that case a negative background doesn't make sense, and won't work with a logR transform. Oversubtraction of background is probably not an issue for XRR?

One thing I've thought about is that if you know your background has to be positive, then it's numerically more stable in a fit to have the background specified in log-space, i.e. limits=[-8, -5], rather than in lin-space, limits=[10**-8, 10**-5].
I wonder if it's still possible to have the magnitude of the background specified in log-space if the background can be negative.

@bmaranville
Copy link
Member Author

bmaranville commented Nov 7, 2024

The limits proposed here are "outer limits", in the sense that our model classes apply them to eliminate parameter values that are known to be unphysical, like negative thickness or roughness.

Users apply their own limits (which we term "bounds") when setting up the fit, and a combined effective limit (whichever is most restrictive) is passed to the fitting engines.

We don't currently have a way to specify limits in log-space. We could parameterize background on a log scale, but then we'd have the deal with the edge case of "negative" apparent background, which is the motivation for this PR in the first place! :)

@bmaranville bmaranville merged commit cefc166 into master Nov 20, 2024
10 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.

5 participants