Fixed Exxx lint issues

[Sindhu-Vasireddy]

Description

Fixed the Exxx lint issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Issue Number

#275
Fixed the Exxx issues - E741,E501,E721,E722 and added ignore for SIM108

Code Compatibility

• I have performed a self-review of my code

Code Performance and Testing

• I ran the uv run train and (if necessary) uv run evaluate on a least one GPU node and it works
• If the new feature introduces modifications at the config level, I have made sure to have notified the other software developers through Mattermost and updated the paths in the $WEATHER_GENERATOR_PRIVATE directory

Dependencies

• I have ensured that the code is still pip-installable after the changes and runs
• I have tested that new dependencies themselves are pip-installable.
• I have not introduced new dependencies in the inference portion of the pipeline

Documentation

• My code follows the style guidelines of this project
• I have updated the documentation and docstrings to reflect the changes
• I have added comments to my code, particularly in hard-to-understand areas

Additional Notes

[FussyDuck]

All committers have signed the CLA.

[grassesi]

Looks good to me, although personally I am not a big fan of the line-length being increased. Feel free to address my comments at your own discretion.

[grassesi]

What is the rationale here for increasing the line length?

[clessig]

Agreed, 100 seems better

[Sindhu-Vasireddy]

I will reset the length back to 100 and update.

[grassesi]

Would be great if you actually only catch relevant exceptions (probably FileNotFound, maybe ValueError?)

[Sindhu-Vasireddy]

I wanted to catch specific codes but the problem in enlisting only the specific exceptions was for instance if there were not so common PermissionError or some FormatErrors it would not be caught, so instead I wanted to catch the general exception, if needed can print the stack trace for debugging as well...

[Sindhu-Vasireddy]

Can catch common exceptions if that is preferred and add another general exception catch for those missed by first catch?

[grassesi]

I would advice against a general exception catch (since usually you want to know when something breaks). To handle the more uncommon exceptions you can handle them all at once like so:

try:
    dangerous_code()
except (Exception1, Exception2, ...) as e:
    handle_exceptions()

[Sindhu-Vasireddy]

yes, I understand but this is what I meant or suggested as an alternative which is as below, to not discard general exception altogether missed by the first catch, as it is very much possible to enumerate n exceptions and miss something which will not be caught.

try: dangerous_code() except (PermissionError, OSError, UnicodeDecodeError) as e: handle_known_issue(e) except Exception as e: log_unexpected_error(e) raise

I have now caught format and file based errors in two catches giving specific context and one general exception catch block for anything else.

[grassesi]

see above

[grassesi]

See above

[tjhunter]

@Sindhu-Vasireddy thanks for the changes. Regarding figuring out the exceptions: the paid version of copilot for vscode (10 eur / month) is very helpful at autocompleting the exceptions (or you can copy/paste the try/catch block in your favorite LLM and prompt it to give the most likely exceptions)

[tjhunter]

good call, it can indeed make the code more readable

[tjhunter]

can you also change the print statement to _logger.warn(...)? Another task is also to remove all the print statements

[clessig]

But let's remove the print() in another PR :)

[Sindhu-Vasireddy]

Also as per @tjhunter's comment only for these 3 try catch blocks I have set the print to logger.error for the two file processesing errors and general exception, kept the initial format errors as logger.warn.

If you would like me to remove these I can do so as well. Kindly let me know your preference

[tjhunter]

Since a few changes are still required, I am marking this PR with changes requested.

[Sindhu-Vasireddy]

Will make the changes and update the PR

Since a few changes are still required, I am marking this PR with changes requested.

[Sindhu-Vasireddy]

Addressed the comments and resolved conflicts, could you kindly re-review

[grassesi]

Looks good to me, my main concerns are addressed. However I noticed some commented code fragments (probably introduced somewhere else). @clessig Do you know when they were introduced and if this is outdated or relevant code? It should be addressed but for the sake of getting this merged maybe in a separate issue?

[grassesi]

I know its not your code originally, but commented out code should be removed: It is unknown if this code is still functional, and if so it is also remembered by git => There is no need to comment it out just to remember it. @clessig can you maybe comment on if this is an artifact that should be removed?

[clessig]

I don't buy the second argument: It's remembered by git doesn't help if it's not remembered by any human. And humans need hints or triggers to remember things.

The concrete case here is Linear versus MLP for the embedding, with the MLP being commented out. This has never been properly ablated. That's why it's commented out for someone to pick up. An issue would probably be a better way as a trigger.

[tjhunter]

The issue is that this commented out code looks like random garbage in the middle of the rest of the code, and there is no limit to it: if someone starts to put such lines, other people would feel entitled to put full files. My suggestion would be to not have it as a comment. An alternative is using conditional statement, ideally with a config flag eventually:

unembed_layer = "linear"
if unembed_layer == "linear":
	l = [torch.nn.Linear(dim_embed, dim_out) for _ in range(num_channels)]
else:
	l = [torch.nn.Sequential(...) for _ in range(num_channels)]
self.unembed = torch.nn.ModuleList(l)

[grassesi]

See above

[clessig]

The code should be kept since the work around below is very difficult to read, and this code is what explains what is actually happening. What is missing is a comment (and perhaps also in the comment about the work around).

[grassesi]

see above

[clessig]

The comment explains why it's there. This needs to be parametrized, but it would be worthwhile to also investigate what triggers the issues and if there's a better workaround.

[grassesi]

See above

[clessig]

Comment above explains it. Any issue should be open to investigate the problem. I would keep the code in the meantime.

[grassesi]

:)

[grassesi]

:)

[grassesi]

:)

[tjhunter]

The issue is that this commented out code looks like random garbage in the middle of the rest of the code, and there is no limit to it: if someone starts to put such lines, other people would feel entitled to put full files. My suggestion would be to not have it as a comment. An alternative is using conditional statement, ideally with a config flag eventually:

unembed_layer = "linear"
if unembed_layer == "linear":
	l = [torch.nn.Linear(dim_embed, dim_out) for _ in range(num_channels)]
else:
	l = [torch.nn.Sequential(...) for _ in range(num_channels)]
self.unembed = torch.nn.ModuleList(l)

[Sindhu-Vasireddy]

@tjhunter I have now rebased and linted the latest changes as well. Could you kindly re-review.

[tjhunter]

below required-version in the same file, can you add the following:

[tool.ruff.format]
# Use Unix `\n` line endings for all files
line-ending = "lf"

[tjhunter]

@Sindhu-Vasireddy thank you very much! I am merging.