Skip to content

reformat #48

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

Merged
merged 4 commits into from
Mar 24, 2025
Merged

reformat #48

merged 4 commits into from
Mar 24, 2025

Conversation

ClashLuke
Copy link
Collaborator

I'm not happy with many of ruff's formatting changes and had to change/remove some of the tests. This may be a good opportunity to go through the unit tests again and ensure everything works in the new API.

@francois-rozet
Copy link
Collaborator

francois-rozet commented Mar 3, 2025

Ruff's formatter is based on Black, which has become the standard formatter for Python. The philosophy of Black is to have as little parameters as possible; personal preferences should be avoided, as they are incompatible with consistency. Black's formatting choices are generally pragmatic, favoring readability over compactness.

For example, in the current code base, the following pattern

function(lots, of, arguments,
         and, even, more, arguments)

has several drawbacks

  1. changing the name of the function means re-aligning all arguments.
  2. adding an argument leads to re-aligning all following arguments.
  3. if the name of the function is long, a lot of horizontal space is lost, defeating the purpose of using a line return.

I actually don't like all the choices of Black either, but consistency is worth the price.

@ClashLuke
Copy link
Collaborator Author

Hm, I'd rather realign the arguments (Ctrl-Shift-Alt-L in PyCharm) than spend 90% of the screen on whitespace with

function(
  lots,
  of,
  arguments,
  and,
  even,
  more,
  arguments,
  ):

It's definitely more consistent, especially with type hints. However, it'd also make maintenance uncomfortable with half the file being function definitions.

As far as I can tell, these choices are on purpose and should invite you to stop using functions with many arguments, but the often-recommended alternatives (like context objects) aren't necessarily more readable either.

To be clear, I'm fully aware that the current 10+ layers of abstractions aren't friendly to debug or maintain, so, ideally, we should find a solution for that, too.

@francois-rozet
Copy link
Collaborator

francois-rozet commented Mar 4, 2025

If there are not too many such cases, you could consider using # fmt: off for the code you want to be more compact.

I also think that using cascading keyword arguments could help with reducing the number of function arguments.

namgyu-youn referenced this pull request in namgyu-youn/HeavyBall Mar 11, 2025
@ClashLuke
Copy link
Collaborator Author

I've further re-enabled opt-einsum. It was previously disabled due to a bug on PyTorch's end, where many functions couldn't be compiled. This appears to be fixed now, so we can re-enable it.

@ClashLuke ClashLuke merged commit 9aed3c0 into main Mar 24, 2025
1 check passed
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