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

Line too long #424

Merged
merged 16 commits into from
Oct 29, 2024
Merged

Line too long #424

merged 16 commits into from
Oct 29, 2024

Conversation

evidencebp
Copy link
Contributor

No description provided.

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 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (190308e) to head (18c9b96).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #424   +/-   ##
=======================================
  Coverage   98.78%   98.78%           
=======================================
  Files          28       28           
  Lines        1982     1982           
  Branches      354      354           
=======================================
  Hits         1958     1958           
  Misses          2        2           
  Partials       22       22           

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

Copy link
Collaborator

@xiki-tempula xiki-tempula left a comment

Choose a reason for hiding this comment

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

LGTM

@xiki-tempula xiki-tempula enabled auto-merge (squash) October 29, 2024 18:33
@xiki-tempula xiki-tempula merged commit 9e8139e into alchemistry:master Oct 29, 2024
7 of 8 checks passed
@evidencebp
Copy link
Contributor Author

Thanks, @xiki-tempula !

evidencebp added a commit to evidencebp/pylint-intervention that referenced this pull request Oct 30, 2024
jaclark5 pushed a commit to jaclark5/alchemlyb that referenced this pull request Nov 15, 2024
* docs\conf.py  line-too-long

Made line shorter, its end wasn't readable

* src\alchemlyb\visualisation\dF_state.py  too-many-branches

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.

* src\alchemlyb\workflows\abfe.py  too-many-branches

Both methods generate_result and estimate had a bit too many branches.
I extracted methods to reduce complexity and make code clearer.

* src\alchemlyb\parsing\namd.py  too-many-branches

Two functions, extract_u_nk and _get_lambdas had too many branches. Smaller functions were extracted.

* src\alchemlyb\parsing\amber.py  line-too-long

Made two readable lines shorter

* src\alchemlyb\parsing\util.py   line-too-long

Made a readable line shorter

* src\alchemlyb\visualisation\ti_dhdl.py  too-many-statements

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.

* Add default value for mnb in get_determine_orientation

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.

* Fix _fit_estimators extraction

_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

* Revert "Fix _fit_estimators extraction"

This reverts commit f017972.

* Revert "Add default value for mnb in get_determine_orientation"

This reverts commit a1ea9d9.

* Revert "src\alchemlyb\visualisation\ti_dhdl.py  too-many-statements"

This reverts commit d2ed7fa.

* Revert "src\alchemlyb\parsing\namd.py  too-many-branches"

This reverts commit 7ece0f8.

* Revert "src\alchemlyb\workflows\abfe.py  too-many-branches"

This reverts commit ea1ecca.

* Revert "src\alchemlyb\visualisation\dF_state.py  too-many-branches"

This reverts commit 3a311d4.

---------

Co-authored-by: Zhiyi Wu <zwu@exscientia.ai>
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