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

Pylint alerts corrections as part of intervention experiment #420

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

evidencebp
Copy link
Contributor

Makes the interventions describe in intervention issue.

Each intervention was done in a dedicated commit with a message explaining it.

Some of the change are too long lines and they are minor.

In cases of too many barnches or statments, I extracted functions to reduce the compelxity.
These are done in the files: src\alchemlyb\visualisation\dF_state.py, src\alchemlyb\workflows\abfe.py, src\alchemlyb\parsing\namd.py, src\alchemlyb\visualisation\ti_dhdl.py

Made line shorter, its end wasn't readable
Function plot_dF_state had 42 branches, while it is recommended not to have more than 12.

The function is structured with clear blocks by logic and documentation.
I extracted methods to encapsulate the logical blocks, reducing complexity.
Both methods generate_result and estimate had a bit too many branches.
I extracted methods to reduce complexity and make code clearer.
Two functions, extract_u_nk and _get_lambdas had too many branches. Smaller functions were extracted.
Made two readable lines shorter
Made a readable line shorter
The function plot_ti_dhdl had 81 statements, while it is recommended not to have more than 50.
I extracted few smaller functions, reducing the length and increasing structure.
The value of mnb is not set in the case of landscape orientation.
It was like that even before extracting get_determine_orientation.
However, now that this is a separated function, the test fails due to returning an unset variable.

This might detect a bigger bug regarding not setting it unintentionally.
_fit_estimators
fails on
E       UnboundLocalError: cannot access local variable 'u_nk' where it is not associated with a value

Since the goal is to reduce branches I revert that and extract the more isolated check_estimators_availability instead
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 98.94737% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.82%. Comparing base (30ecce0) to head (f017972).

Files with missing lines Patch % Lines
src/alchemlyb/visualisation/dF_state.py 97.70% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   98.78%   98.82%   +0.03%     
==========================================
  Files          28       28              
  Lines        1982     2035      +53     
  Branches      436      355      -81     
==========================================
+ Hits         1958     2011      +53     
  Misses          2        2              
  Partials       22       22              

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

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.

1 participant