Skip to content

[ENH] OWFFT: Simplify auto dx handling#810

Merged
stuart-cls merged 10 commits intoQuasars:masterfrom
stuart-cls:owfft-simplify-hene
Jun 2, 2025
Merged

[ENH] OWFFT: Simplify auto dx handling#810
stuart-cls merged 10 commits intoQuasars:masterfrom
stuart-cls:owfft-simplify-hene

Conversation

@stuart-cls
Copy link
Member

@stuart-cls stuart-cls commented May 23, 2025

Following @ngergihun comment in #809 (comment) took the opportunity to simplify our (auto) dx handling.

  • Checkbox is now dx_auto which allows user to override any default value
  • dx default value is set from data table if dx_auto is set, otherwise it's left as a user setting.
  • If no dx is present in the dataset and dx_auto is set, the value falls back to our old DEFAULT_HENE

This gets rid of a bunch of weirdness and generally lets users do what they want.

Still outstanding:

Comments welcome.

@ngergihun
Copy link
Contributor

ngergihun commented May 25, 2025

Thanks for this upgrade, @stuart-cls. It is much more convenient to use this way. I like it a lot.

One addition might help reduce users' confusion. Maybe it breaks the info box standardization a bit, but it would probably be nice to notify the user that the automatically considered laser is the HeNe because there was no data about point spacing. And not just displaying the wavenumber. Even I was confused for a minute about what was happening, and I had to check the code to understand.

Something like:

if lwn == DEFAULT_HENE:  
      self.infoc.setText("Dataspacing not found: auto HeNe {0}, {1} sampling interval".format(lwn, udr))  
else:  
      self.infoc.setText("{0} cm<sup>-1</sup> laser, {1} sampling interval".format(lwn, udr))

However, the standardization of the messages is another aspect, so I don't know if it fits. @borondics always has some concepts about UI improvements, so he probably can provide some good ideas.

For Document what dx is ( yes, it's the pathlength step size), can we use a setToolTip type hover info? For the label or the entire Input Data box?

@stuart-cls
Copy link
Member Author

Thanks, I like both these ideas. I'll add them.

Removes special case of default HeNe (except as the original default).
...except in case of "Calculated Datapoint Spacing"

NB: disabledBy doesn't work for this use-case, need enabledBy instead
@stuart-cls stuart-cls force-pushed the owfft-simplify-hene branch from 87566dc to ed64789 Compare May 26, 2025 19:45
@stuart-cls stuart-cls marked this pull request as ready for review May 26, 2025 21:50
@stuart-cls
Copy link
Member Author

@ngergihun Please take another look.

@ngergihun
Copy link
Contributor

Hi @stuart-cls !
I think the info is much more descriptive and better for following what is going on.
I have got an error for the settings migration:
image

I guess this is because the dx_HeNe was removed and is not in the settings anymore.

@stuart-cls
Copy link
Member Author

@ngergihun Interesting, thanks! I thought the version check would catch that but there must be some edge case I don't know about. Settings are tricky :)

@markotoplak Can you take a quick look at the settings migration I'm doing?

@stuart-cls stuart-cls requested a review from markotoplak May 28, 2025 15:32
@markotoplak
Copy link
Collaborator

@ngergihun, did you perhaps test migration with one the workflows that was saved from this branch but before migration PR was added? That would explain the error. Otherwise, @stuart-cls, I do not understand why that code would crash.

@ngergihun
Copy link
Contributor

@ngergihun, did you perhaps test migration with one the workflows that was saved from this branch but before migration PR was added? That would explain the error.

Ohh, yes, sorry. This was the problem. Reset widget settings solved the problem. Sorry for the false alarm.

@stuart-cls stuart-cls merged commit 3dcee9c into Quasars:master Jun 2, 2025
19 of 20 checks passed
@stuart-cls
Copy link
Member Author

Thanks all!

@stuart-cls stuart-cls deleted the owfft-simplify-hene branch June 2, 2025 15:38
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.

3 participants