Skip to content

Add a default value for postprocessing#218

Merged
gdalle merged 4 commits intomainfrom
postprocessin_no_kw
Mar 31, 2025
Merged

Add a default value for postprocessing#218
gdalle merged 4 commits intomainfrom
postprocessin_no_kw

Conversation

@amontoison
Copy link
Copy Markdown
Collaborator

@amontoison amontoison commented Mar 31, 2025

I checked something a few days with @code_warntype and I found that the compiler generated a very complex code to handle the lack of default value for the keyword argument.

It could also be an argument.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b33d436) to head (97c602c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #218   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1661      1661           
=========================================
  Hits          1661      1661           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Mar 31, 2025

Actually, not putting a default value in the inner method was a conscious choice. It helps prevent situations where you forget to pass the keyword argument from the outer method, thus ending up with a default value you didn't choose.
We can make it a positional argument but honestly this is so far down the list of suboptimalities that I don't think it matters (otherwise JET would have yelled at us)

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Mar 31, 2025

And there's also an upside to having a setting always be a keyword or positional argument throughout the code, instead of switching. Keyword arguments are there to simplify writing, so it's okay to use them

Copy link
Copy Markdown
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

I think this doesn't change anything and I'd rather preserve the current behavior

@amontoison
Copy link
Copy Markdown
Collaborator Author

Is ok to use postprocessing directly as an argument? We only call these functions in interface.jl and I don't the gain to use optional argument or position argument for it.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Mar 31, 2025

Yes it is okay, my objection was two-fold:

  1. it changes the logic of inner calls compared to outer calls
  2. it brings no measurable performance benefit

Of course both are rather minor objections in this specific case, which is a very simple PR, but I'm mentioning them anyway.

If we don't try to maintain a simple and readable library, we will end up like ColPack.
I'm all for optimizing things which we know have a performance impact. But that requires profiling bottlenecks first, and working on those.

@gdalle
Copy link
Copy Markdown
Member

gdalle commented Mar 31, 2025

Essentially I'm protesting the spirit of the PR, more than the specific code inside it ^^

@amontoison
Copy link
Copy Markdown
Collaborator Author

I just removed the need to specify postprocessing, which was mandatory because no default value where available.
I think it is good for everyone now (user and compiler).

Copy link
Copy Markdown
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

Again, I'm not saying this is a bad idea, I'm saying this is a very you idea ;)

@gdalle gdalle merged commit 8b18bfd into main Mar 31, 2025
6 checks passed
@amontoison amontoison deleted the postprocessin_no_kw branch March 31, 2025 19:43
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