Skip to content

Address flake8 error lambda functions (E731) #78

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
Closed
wants to merge 1 commit into from
Closed

Address flake8 error lambda functions (E731) #78

wants to merge 1 commit into from

Conversation

bobleesj
Copy link

@bobleesj bobleesj commented Aug 21, 2024 •
edited
Loading

Tests pass:

imac@imacs-iMac diffpy.srfit % python -m diffpy.srfit.tests.run
WARNING:diffpy.srfit.tests:No module named 'sas', SaS tests skipped.
WARNING:diffpy.srfit.tests:Cannot import pyobjcryst, pyobjcryst tests skipped.
WARNING:diffpy.srfit.tests:Cannot import diffpy.srreal, PDF tests skipped.
......ssss................................sssssssssss....ssssss...........................sss..s..............
----------------------------------------------------------------------
Ran 110 tests in 0.170s

OK (skipped=25)

bobleesj
bobleesj commented Aug 21, 2024
View reviewed changes
Copy link
Author

@bobleesj bobleesj left a comment

Choose a reason for hiding this comment

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

Please review @sbillinge

src/diffpy/srfit/sas/prcalculator.py
@@ -92,7 +92,7 @@ def __call__(self, r):
self._invertor.y = iq
self._invertor.err = diq
c, c_cov = self._invertor.invert_optimize()
calculate_pr = lambda x: self._invertor.pr(c, x)
Copy link
Author

Choose a reason for hiding this comment

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

I think using lambda functions that are short and concise actually increase readability... Hence adding # noqa: E731 as well.

@bobleesj
Copy link
Author

This is the last PR for flake8. We shall implement CI/pre-commit very soon.

@bobleesj bobleesj marked this pull request as ready for review August 21, 2024 02:16
sbillinge
sbillinge reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@sbillinge sbillinge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please see comments. I am not a fan of lots of lambdas. I like them for sort keys and things like that where they are much more readable having the logic in place (not assigned) but elsewhere I think they rarely make things clearer than clearly written code with explicit functions and well chosen function names

src/diffpy/srfit/equation/builder.py
@@ -661,7 +661,7 @@ def __wrapSrFitOperators():
opmod = literals.operators
excluded_types = set((opmod.CustomOperator, opmod.UFuncOperator))
# check if opmod member should be wrapped as OperatorBuilder
is_exported_type = lambda cls: (
is_exported_type = lambda cls: ( # noqa: E731
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find this very readable. We can't do better here?

src/diffpy/srfit/sas/prcalculator.py
@@ -92,7 +92,7 @@ def __call__(self, r):
self._invertor.y = iq
self._invertor.err = diq
c, c_cov = self._invertor.invert_optimize()
calculate_pr = lambda x: self._invertor.pr(c, x)
calculate_pr = lambda x: self._invertor.pr(c, x) # noqa: E731
Copy link
Contributor

Choose a reason for hiding this comment

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

This one also better to put it in the map directly? Or maybe as an explicit function

src/diffpy/srfit/tests/testbuilder.py
@@ -170,7 +170,7 @@ def testParseEquation(self):
sigma = 0.1
eq.x.setValue(x)
eq.sigma.setValue(sigma)
f = lambda x, sigma: sqrt(e ** (-0.5 * (x / sigma) ** 2))
f = lambda x, sigma: sqrt(e ** (-0.5 * (x / sigma) ** 2)) # noqa: E731
Copy link
Contributor

Choose a reason for hiding this comment

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

Here in the tests they are quite readable and not doing much harm so I suggest leaving with noqa

@bobleesj
Copy link
Author

Please see comments. I am not a fan of lots of lambdas. I like them for sort keys and things like that where they are much more readable having the logic in place (not assigned) but elsewhere I think they rarely make things clearer than clearly written code with explicit functions and well chosen function names

Noted. I will address these later in the afternoon.

@bobleesj
Copy link
Author

@sbillinge I will close this PR and make separate PRs address each file or so. I will refer back to your in-line comments.

@bobleesj bobleesj closed this Aug 21, 2024
@bobleesj bobleesj mentioned this pull request Aug 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbillinge sbillinge sbillinge left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobleesj @sbillinge