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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in to_raman_shift #140

Merged
merged 4 commits into from
Sep 23, 2022
Merged

Fix bugs in to_raman_shift #140

merged 4 commits into from
Sep 23, 2022

Conversation

jlaehne
Copy link
Contributor

@jlaehne jlaehne commented Sep 6, 2022 •
edited

Description of the change

The conversion to Raman shift (relative wavenumber) is not working as it should:

  • data was inverted compared with axis vector
  • label axis as Raman Shift
  • make Jacobian=False default
  • inplace=False was working with original data instead of copy

Progress of the PR

  • Change implemented (can be split into several points),
  • added tests,
  • added line to CHANGELOG.md,
  • ready for review.

@jordiferrero
Copy link
Contributor

Is this ready for review?

@jlaehne
Copy link
Contributor Author

jlaehne commented Sep 19, 2022

Is this ready for review?

I still have to fix the tests. Also currently, it runs the to_invcm function, which inverts the data order and then inverts it back ... I want to see if it does not make sense to change that. But as it is a serious bug, I should probably finish the fix first and a smoother solution could come later.

@jlaehne jlaehne marked this pull request as ready for review September 21, 2022 21:43
jordiferrero
jordiferrero reviewed Sep 22, 2022
View reviewed changes
lumispy/signals/luminescence_spectrum.py
# create and return new signal
else:
s2 = self.to_invcm(inplace=inplace, jacobian=jacobian)
s2.axes_manager.set_axis(
invcmaxis, self.axes_manager.signal_axes[0].index_in_axes_manager
)
s2.data = s2.isig[::-1].data
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to not have to repeat the same lines of code on both sides of the if statement (code not DRY)? What if you set your copy as your signal in this case of inplace=True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably an if/else statement could decide whether to operate on a copy of the signal or a pointer to self ... I would however leave that consolidation to a separate PR. I will open an issue.

lumispy/tests/utils/test_axis-conversion.py Outdated Show resolved Hide resolved
@jlaehne jlaehne mentioned this pull request Sep 22, 2022
Open
2 tasks
@jlaehne jlaehne added this to the v0.2.1 milestone Sep 23, 2022
@jlaehne jlaehne merged commit 838c9e7 into LumiSpy:main Sep 23, 2022
@jlaehne jlaehne deleted the wavenumber branch September 26, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordiferrero jordiferrero jordiferrero approved these changes

Assignees
No one assigned
Labels
type: bug-fix
Projects
None yet
Milestone
v0.2.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jlaehne @jordiferrero