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

Fix signal scaling and add background fit to fitsignal plots #78

Merged
merged 11 commits into from
Mar 12, 2021

Conversation

luisfabib
Copy link
Collaborator

This PR fixes issues related to the plot() method of fitsignal's output. The experimental dipolar signals and the corresponding fit were being plotting with different scales seeming like one of the two was not being plotted.

Potentially closes #74

Changelog:

  • Fitted dipolar signal and experimental data now are plotted with the same scales (the one of the original data).
  • Uncertainty quantification of the dipolar signal fit now properly accounts for the scaling of the signal and displays properly.
  • Background (scaled by its unmodulated contribution) is now plotted along with its uncertainty with the plot() method.
  • Added option to request scaled background instead of the unscaled one as the fit.B output

@luisfabib luisfabib added enhancement New feature or request bugfix Patches something that isn't working labels Feb 8, 2021
@luisfabib luisfabib linked an issue Mar 1, 2021 that may be closed by this pull request
@stestoll
Copy link
Collaborator

stestoll commented Mar 5, 2021

Instead of introducing a new input parameter to request the scaled background signal, it might be better to just return both unscaled and scaled background in the FitResult structure. Same for the overall signal. That is the easiest from the user perspective, as they don't have to remember yet another input parameter, and they can just look at FitResult and then pick what they want for plotting etc.

@luisfabib
Copy link
Collaborator Author

This would be an excellent idea, will do so. Indeed it is easier to search for outputs than to prepare inputs and options for certain behavior.

It would also go in the way of a recent request:

Couldn't we/you solve that in a way, where the output 'fits' to the input in a future version? Or was that fixed as well? :-)
Originally posted by @laenan8466 in #74 (comment)

@luisfabib
Copy link
Collaborator Author

The recent changes should now close #86 as well.

@luisfabib luisfabib linked an issue Mar 11, 2021 that may be closed by this pull request
3 tasks
@luisfabib luisfabib merged commit 2241841 into main Mar 12, 2021
@stestoll stestoll deleted the fix/fitsignal_plot branch March 19, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Patches something that isn't working enhancement New feature or request
Projects
None yet
2 participants