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

Bnb/h5 with era training #162

Merged
merged 15 commits into from
Aug 29, 2023
Merged

Bnb/h5 with era training #162

merged 15 commits into from
Aug 29, 2023

Conversation

bnb32
Copy link
Collaborator

@bnb32 bnb32 commented Aug 24, 2023

No description provided.

@bnb32 bnb32 marked this pull request as ready for review August 24, 2023 18:36
@bnb32 bnb32 force-pushed the bnb/h5_with_era_training branch 2 times, most recently from 671eee9 to 761e8e1 Compare August 25, 2023 13:56
@bnb32 bnb32 requested a review from grantbuster August 25, 2023 13:58
shape=self.shape,
val_split=0.0,
**bias_handler_kwargs,
)
Copy link
Member

Choose a reason for hiding this comment

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

Did you run an auto formatter on everything? Do you like this style better and why? Some thoughts: The original formatting has strong horizontal separation of the output var, method, and input args. In the original formatting, it's very clear which lines are part of the function call. In the new formatting, it's more difficult to visually separate the output arg from the input args. Lastly, what is the logic for having the closing parenthesis on the 0 column? It's closing the function call, not the original line?

Copy link
Member

Choose a reason for hiding this comment

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

@bnb32 it looks like some of the files are still black-formatted. Are you going to revert this or are we going with the black style?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I missed some. I have to revert manually which I will do if you want. Although after playing around with a bunch of formatters since the start of this discussion I now like black even more :)

Copy link
Member

Choose a reason for hiding this comment

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

I just think some formatting like in the screenshot below is total garbage (forward_pass.py). It looks like C++ nonsense and i think we're losing the readability of python. Some of this stuff is just the result of chaining too many commands into one "line".

image

Copy link
Collaborator Author

@bnb32 bnb32 Aug 28, 2023

Choose a reason for hiding this comment

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

Yeah I'm not a fan of that formatting either but I don't think it's that frequent and I personally consider the consistency a net positive. I think

out_arg = func(
   input_args
)

is quite readable in general though.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a step backwards as far as code standards but if you want to do it for this repo i'll leave that decision to you. I think it's worst with massive chained "one-liners" like in the screenshot above. Basically i think this moves the onus to the developer to write smaller function chains to get more sensible code (e.g., split up the code above into 3-4 single lines that are actually single lines). IMO we should have better standards than in the example above. If we do want to do black across the board, i recommend adding the github action: https://github.com/marketplace/actions/run-black-formatter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think that onus is a bad one? I think the example above is the worst and for 99% the readability is maintained or improved tbh. I've turned off black formatting though since I don't want to enforce something you don't like, but I'd prefer not to have to manually revert a ton.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think moving the focus onto a different type of style is bad. I do think black makes some code more readable and some other code much less so. I guess it just goes to show that these tools won't automatically transform any code into "good" code. There's still a lot of human considerations the developer has to work with to make readable code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely agree!

sup3r/preprocessing/data_handling/dual_data_handling.py Outdated Show resolved Hide resolved
sup3r/preprocessing/data_handling/dual_data_handling.py Outdated Show resolved Hide resolved
sup3r/preprocessing/dual_batch_handling.py Show resolved Hide resolved
sup3r/preprocessing/dual_batch_handling.py Show resolved Hide resolved
@bnb32 bnb32 force-pushed the bnb/h5_with_era_training branch 4 times, most recently from aa8cff8 to feea8c0 Compare August 28, 2023 22:11
np.maximum(0, (self.strategy.spatial_pad - s1_start)))
pad_s1_end = (self.strategy.spatial_pad + s1_stop
- self.strategy.grid_shape[0])
pad_s1_end = int(np.maximum(0, pad_s1_end))
Copy link
Member

Choose a reason for hiding this comment

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

Thank you! 🙌

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing!

@bnb32 bnb32 merged commit fb000fa into main Aug 29, 2023
5 checks passed
@bnb32 bnb32 deleted the bnb/h5_with_era_training branch August 29, 2023 18:24
github-actions bot pushed a commit that referenced this pull request Aug 29, 2023
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