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 masked TOF columns in reduction #173

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

GuiMacielPereira
Copy link
Collaborator

@GuiMacielPereira GuiMacielPereira commented Feb 26, 2025

Description of work:
I noticed that when columns of zeros were present the fit of the spectra were failing.
Then after some time I spotted a mistake in the calculation with the error function.
Previous unit tests were not picking this up because I had written the error into them.
I edited the unit tests to now always expect the correct result.

To test:
Code review, check that all tests pass (the expected behaviour is now covered by the unit test)

Previous error function was calculating the nonzero values in an incorrect manner.
this is because I was modifying data_y array and then using it again with np.nonzero().
This was clearly a blunder on my part and was producing bad fits when some values are masked with zeros.
@GuiMacielPereira GuiMacielPereira added bug Something isn't working vesuvio-analysis labels Feb 28, 2025
@GuiMacielPereira GuiMacielPereira added this to the v6.13 milestone Feb 28, 2025
@@ -532,9 +532,10 @@ def _error_function(self, pars):
data_e = self._dataE[self._row_being_fit]

# Ignore any masked values on tof range
ncp_total = ncp_total[np.nonzero(data_y)]
data_y = data_y[np.nonzero(data_y)]
data_e = data_e[np.nonzero(data_y)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My previous mistake was that I was doing np.nonzero(data_y) when I just modified data_y in the previous line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vesuvio-analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant