Skip to content

adaptation for raw data file reading and minor fit routine modifications#28

Merged
mohamedfaresslim merged 9 commits intomainfrom
fit_minor_modifs
Mar 15, 2022
Merged

adaptation for raw data file reading and minor fit routine modifications#28
mohamedfaresslim merged 9 commits intomainfrom
fit_minor_modifs

Conversation

@mohamedfaresslim
Copy link
Copy Markdown
Contributor

Modifications done:

  1. Adaptation of the two routines for calibration (angle and ch --> energy) to read correctly from the raw data file
  2. Minor modifications in the fitEDD function to not crash if no convergence of the curve-fit function --> try and except loops were added for that. The solution is set to a matrix of ones if no convergence.

Copy link
Copy Markdown
Collaborator

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

I will have a look at the failing tests

Comment thread easistrain/EDD/angleCalibEDD.py Outdated
Comment thread easistrain/EDD/calibrationEDD.py Outdated
Comment thread easistrain/EDD/fitEDD.py
optimal_parametersVD = np.ones_like(initialGuessVD)
covarianceVD = np.ones(
(5 * nbPeaksInBoxes[i], 5 * nbPeaksInBoxes[i])
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would need more info on the problem you want to solve here.

Is the problem that if a fit fails, it completely stops the program ? I don't know if having ones as parameters/covariance is the best way to communicate that a fit has failed. Perhaps we should instead completely skip the saving of the fit ?

In any case, I think it is worth putting a print to warn the user. Something like:

print(f"Peak {i} could not be fitted ! Filling fit parameters with ones")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Q: Is the problem that if a fit fails, it completely stops the program ?
A: Yes.

Q: Perhaps we should instead completely skip the saving of the fit ?
A: It can be done like that also but then we have to adapt all the rest of the function to that modification. The idea by putting a matrix of ones is that if the fit fails the user of the routine will see immediately that there is something wrong with that point when plotting the results for example.

In any case, I think it is worth putting a print to warn the user --> Totally agree with you for this point. We must do it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All right. What about putting something more explicit such as NaN or zeros instead of ones ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Zeros, I would avoid because it can cause some other issues for the next tasks as for example dividing by zero ....
NaN, it can also be an option but I do not know if it could also crush the next steps if we want to do operations on NaN values.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Normally, operations on NaN will yield NaN. Which is good because we will be able to know in subsequent steps that a fit failed.

Then, at some point in the processing (when regrouping?), we should filter out NaN to avoid processing the data where the fit failed.

Comment thread easistrain/EDD/angleCalibEDD.py Outdated
Comment thread easistrain/EDD/fitEDD.py
Copy link
Copy Markdown
Collaborator

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

👍

@mohamedfaresslim mohamedfaresslim merged commit 152de20 into main Mar 15, 2022
@mohamedfaresslim mohamedfaresslim deleted the fit_minor_modifs branch March 15, 2022 10:23
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.

2 participants