-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add reader for Jobin-yvon .xml-format #25
Conversation
Codecov ReportBase: 82.87% // Head: 83.37% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 82.87% 83.37% +0.50%
==========================================
Files 40 40
Lines 8041 8337 +296
Branches 1860 1930 +70
==========================================
+ Hits 6664 6951 +287
+ Misses 913 911 -2
- Partials 464 475 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@LMSC-NTappy as you have implemented the |
rsciio/jobin_yvon/api.py
Outdated
name: Str | ||
Name of the axis. | ||
""" | ||
scale = np.abs(array[0] - array[-1]) / (array.size - 1) |
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 like the approach of loading as uniform axis and warn the user of possible non-uniformity.
However, I would use a regression method such as numpy.polyfit in order to minimize the linearized axis error:
[scale, offset] = np.polyfit(np.arange(array.size),array,1)
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.
An advantage of the method I used is that the boundaries match exactly.
Moreover, the offset obtained from the fit is wrong when the axis contains negative values.
For example there is a test file with navigation axis (-2, 0, 2).
I personally wouldn't use the fit for the offset calculation, but I could imagine using a fit for the scale.
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.
Indeed, the only additional degree of freedom of the fit should be that the first and last point are not fixed. So if we want to keep their positions, there is no advantage of the fit.
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.
An advantage of the method I used is that the boundaries match exactly.
Indeed, the only additional degree of freedom of the fit should be that the first and last point are not fixed. So if we want to keep their positions, there is no advantage of the fit.
I don't see any virtue in ensuring that first/last points match exactly but if that's a concern to you I guess you have good reasons.
Moreover, the offset obtained from the fit is wrong when the axis contains negative values.
For example there is a test file with navigation axis (-2, 0, 2).
Sounds odd to me. Running this gives
np.polyfit(x=np.arange(3),y=[-2,0,2],deg=1)
#out: array([ 2., -2.])
Which is exactly a scale of 2 and an offset of -2
I personally wouldn't use the fit for the offset calculation, but I could imagine using a fit for the scale.
Definitely this would induce more error than what is currently implemented (at lease in some cases)
You do as you please :)
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.
Which is exactly a scale of 2 and an offset of -2
I made a mistake here. Yes it should also work in this case.
I use a fit now to get the scale/offset of the signal axis.
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.
Overall looks good to me.
I have some troubles with the way the non uniform axes are linearized, which can lead to errors in some cases.
Other than that, I suggested a few changes that in my opinion improve readability
Empad and Jobin Yvon share the same extension (.xml).
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.
Looks good to me.
Linearisation strategy is still a subject of disagreement but if by default the signal is loaded as nua I guess it is the user's responsibility to linearize in the way it pleases him/her (and decide how the linearization error is dealt with).
Cheers
Nico
fa32a1e
to
f4b5dc3
Compare
f4b5dc3
to
f1194fc
Compare
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.
Looks good to me this way!
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.
Looks good to me!
IMHO there is nothing preventing this to be merged.
Description of the change
Add support for reading the .xml-format from jobin-yvon.
Progress of the PR
upcoming_changes
folder (seeupcoming_changes/README.rst
),readthedocs
doc build of this PR (link in github checks)Minimal example of the bug fix or the new feature