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

Avoid lambda flake8 error with private function #56

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

bobleesj
Copy link
Contributor

@bobleesj bobleesj commented Aug 2, 2024

As provided in the previous comment: #54 (comment)

For these lambda flake8 errors we would normally see if the function is being used in more than one place, and if it is, define a private function somewhere outside the main part of the code.

I created a private function called _interpolate_stretching_factor in the ComponentSignal class since the function has been used more than once. Thank you for the feedback.

Please confirm my function description.

  • comments removed
  • CI, pre-commit ✅

@bobleesj bobleesj changed the title Avoid lamdbda flake8 erorr with private function Avoid lambda flake8 erorr with private function Aug 2, 2024
@bobleesj bobleesj changed the title Avoid lambda flake8 erorr with private function Avoid lambda flake8 error with private function Aug 2, 2024
Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

I nervous that this is not the right design here. this function seems to be doing a very shallow wrap of np.interpolate. Can't we just call np.interpolate?

Why don't you think about it for a sec and propose something cleaner?

@@ -25,6 +25,23 @@ def __init__(self, grid, number_of_signals, id_number, perturbation=1e-3):
self.stretching_factors = np.ones(number_of_signals) + np.random.randn(number_of_signals) * perturbation
self.id = int(id_number)

def _interpolate_stretching_factor(self, stretching_factor):
"""Interpolates the intensity values over a normalized grid scaled by a given stretching factor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Private functions don't need docstrings

@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 2, 2024

The original func variable containing the lambda expression is used twice within apply_stretch.

First, the derivative of func is computed:

derivative_func = numdifftools.Derivative(func)

Second, the expression is simply computed:

stretched_component = func(self.stretching_factors[m])

Due to the first usage, it is required that we express the np.interp part as a function block.

To me personally, I would have maintined the lambda expression as it was originally. To avoid the flake8 error, I would use # noqa: E731 as recommended.

I am aware using this specific tag to avoid errors should be used as the last resort and it may not be the best practice (in most cases). However, we do not want to have a private function or nested function block either since it's a very simple function.

How is my thought? @sbillinge

@sbillinge
Copy link
Contributor

The original func variable containing the lambda expression is used twice within apply_stretch.

First, the derivative of func is computed:

derivative_func = numdifftools.Derivative(func)

Second, the expression is simply computed:

stretched_component = func(self.stretching_factors[m])

Due to the first usage, it is required that we express the np.interp part as a function block.

To me personally, I would have maintined the lambda expression as it was originally. To avoid the flake8 error, I would use # noqa: E731 as recommended.

I am aware using this specific tag to avoid errors should be used as the last resort and it may not be the best practice (in most cases). However, we do not want to have a private function or nested function block either since it's a very simple function.

How is my thought? @sbillinge

Thanks Bob, great analysis. It is not a huge deal here, so I would probably go with the lambda as you recommend. I might take the opportunity to make the code more readable and call it something other than "func" which is not very descriptive.

@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 7, 2024

Requesting a pull request:

  • func was renamed to interpolate_intensity to improve clarity
  • Added noqa: E731 as discussed

@sbillinge

@sbillinge sbillinge merged commit fb9661e into diffpy:cookie Aug 7, 2024
2 checks passed
@sbillinge
Copy link
Contributor

nicely done !

@bobleesj bobleesj deleted the lambda branch August 7, 2024 12:32
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.

2 participants