Closed matyson closed 1 month ago
Thanks! Backwards compatible on my machine now and tests past locally, just trying to get a sense of what's going on with the CI tests.
This seems a nice addition. I am not sure about the preprocessor name in the widget, though.
So, in the widget, we'd have to present the preprocessor to the user with a name they would expect. Maybe "linear transformation" is too technical. Also, to me, "linear transformation" would imply something linear in the wavenumber range, like we'd achieve with EMSC with 2 parameters. To me this seems more like "stretch and move". What do other tools use?
I am not against "linear transformation" if that is something expected, but in this case, I'd keep the old one, too. Adds almost zero code because we can make a subclass. Also, "Shift spectra" is a bad name because it does not imply direction. :)
And the award for the worst name we already have would go to "Savitzy golay filter". I think we'd need to add a "Derivatives" preprocessor that opens (almost) exactly the same interface.
What do you think, @borondics and @stuart-cls?
@borondics, @stuart-cls, could you decide on how to show this to the user? As I wrote above, I would propose having both Shift and Linear transformations because it makes sense for discoverability. Also, let's decide on naming classes and arguments so that we can finally merge this one.
Hello guys, it is great that we are ending this PR. As far as I understood the last issue is a semantic one. I think we can speed this up as for me the most important part is done. So, let us decide these names. As a suggestion and keeping in mind a general and consistent approach, I would suggest "Vertical Offset" for the existent one (taking the opportunity that Marko is already unhappy with the current name) and "Vertical Scaling" for the new one. The choice is in phase with Marko's comments about the direction.
Please let us know what you want to do next. Do you want them in separate pre-processors or all together in a single box?
We discussed this with @borondics. We decided that replacement with a single preprocessor is natural if we keep the name simple. We decided on the following name: "Linear Transformation" -> "Shift and scale"
We decided on the following naming:
amount
-> offset
Any complaints? :)
We discussed this with @borondics. We decided that replacement with a single preprocessor is natural if we keep the name simple. We decided on the following name: "Linear Transformation" -> "Shift and scale"
We decided on the following naming:
- the argument
amount
->offset
- "Shift Amount" -> "Vertical offset" within the settings
- "Scale Factor" -> "Vertical scaling" within the settings
Any complaints? :)
Sounds good to me!
@raul-freitas great! @matyson, could you please make the changes and then we merge?
We discussed this with @borondics. We decided that replacement with a single preprocessor is natural if we keep the name simple. We decided on the following name: "Linear Transformation" -> "Shift and scale"
We decided on the following naming:
- the argument
amount
->offset
- "Shift Amount" -> "Vertical offset" within the settings
- "Scale Factor" -> "Vertical scaling" within the settings
Any complaints? :)
@markotoplak i think i've made all the suggested changes, could you review it?
Add a scale factor to the CurveShift preprocessor in order to enable scaling the spectra amplitude, now results in a form: res = scale * spectra + amount. Also, rename it into LinearTransform for coherence.