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

Extend shift spectra #718

Merged
merged 7 commits into from
Jul 24, 2024
Merged

Conversation

matyson
Copy link
Contributor

@matyson matyson commented May 3, 2024

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.

Copy link
Member

@stuart-cls stuart-cls left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great.

Since you renamed the preprocessor you will need to do a settings migration, see

if name == "orangecontrib.infrared.rubberband" and version < 2:
name = "orangecontrib.infrared.baseline"
settings["baseline_type"] = 1
version = 2
for an example.

orangecontrib/spectroscopy/widgets/preprocessors/misc.py Outdated Show resolved Hide resolved
@stuart-cls
Copy link
Member

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.

@markotoplak
Copy link
Collaborator

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?

@matyson matyson requested a review from stuart-cls June 13, 2024 12:19
@markotoplak
Copy link
Collaborator

@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.

@raul-freitas
Copy link

raul-freitas commented Jul 11, 2024

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?

@markotoplak
Copy link
Collaborator

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? :)

@raul-freitas
Copy link

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!

@borondics
Copy link
Member

@raul-freitas great! @matyson, could you please make the changes and then we merge?

@matyson matyson requested a review from markotoplak July 12, 2024 17:27
@matyson
Copy link
Contributor Author

matyson commented Jul 17, 2024

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?

@markotoplak markotoplak merged commit 78a85e3 into Quasars:master Jul 24, 2024
6 of 14 checks passed
markotoplak added a commit that referenced this pull request Jul 24, 2024
@matyson matyson deleted the extend-shift-spectra branch July 24, 2024 10:52
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.

5 participants