-
Notifications
You must be signed in to change notification settings - Fork 2
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
Energy calibration bias shift #411
Conversation
I understand that you want to merge to v1 feature branch because these are all breaking changes? |
Yes, they reqire configuration changes |
Pull Request Test Coverage Report for Build 9553353548Details
💛 - Coveralls |
tutorial/2_conversion_pipeline_for_example_time-resolved_ARPES_data.ipynb
Outdated
Show resolved
Hide resolved
This code here unfortunately performs rather poorly. The generation of the per-file data columns alone make the computation ~20% or so slower, adding the energy offset per electron multiplies the binning time by a factor of almost 4! |
Pull Request Test Coverage Report for Build 9587172660Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9587913234Details
💛 - Coveralls |
5f336a8
to
98c4e8c
Compare
Pull Request Test Coverage Report for Build 9588080384Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9628853930Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9628937920Details
💛 - Coveralls |
0004113
to
c347bad
Compare
Pull Request Test Coverage Report for Build 9635909706Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9635950194Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9636169386Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9636247350Details
💛 - Coveralls |
2aaf8b5
to
54f9673
Compare
Pull Request Test Coverage Report for Build 9652315474Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9659018433Details
💛 - Coveralls |
…alibration_bias_shift
…alibration_bias_shift
Pull Request Test Coverage Report for Build 9733951548Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9757007395Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9759204395Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9847465611Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to review but currently, I don't understand the internals of the calibration methods enough to review this properly. I have tried because I wanted to adapt these for the side band correction but failed to understand.
Honestly, It'd be great if a small guide on the internals can be made. We can then also put it in user documentation.
If that's too much, if you could explain why you made these changes, I should be able to review based on that.
The only thing I noticed currently was that the tutorials still have the ref_id (in methods find_bias_peaks etc.) whereas the code only uses the ref_energy now.
tutorial/2_conversion_pipeline_for_example_time-resolved_ARPES_data.ipynb
Show resolved
Hide resolved
Thank you for your effort. I will for now comment on the changes to explain them better. If I find time, I can also make a more general documentation item out of this. |
For using the calibration function for features from sidebands, do the following:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments. Thanks for explaining! Helped a lot. Could be a start to the calibration guide later.
Shift energy calibration by bias voltages, and apply shift of dataset bias upon adding energy column.
Unify dataframe channels config
closes #399
closes #138