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

Construct the grads only once #78

Merged
merged 1 commit into from Nov 27, 2019

Conversation

@eric-wieser
Copy link
Contributor

eric-wieser commented Nov 26, 2019

Before this change, grads were constructed:

  • twice during __init__
  • each time the user called ga.mv() with no arguments
  • each time the user called ga.grads()

After this change, grads are constructed:

  • once during __init__
Before this change, grads were constructed:
* twice during `__init__`
* each time the user called `ga.mv()` with no arguments
* each time the user called `ga.grads()`

After this change, grads are constructed:
* once during `__init__`
@eric-wieser eric-wieser force-pushed the eric-wieser:compute-grads-once branch from 2fa483e to 4ddad82 Nov 26, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #78 into master will decrease coverage by 0.02%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   67.24%   67.21%   -0.03%     
==========================================
  Files           8        8              
  Lines        4817     4816       -1     
==========================================
- Hits         3239     3237       -2     
- Misses       1578     1579       +1
Impacted Files Coverage Δ
galgebra/ga.py 75% <80%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b8055b...4ddad82. Read the comment docs.

Copy link
Member

utensil left a comment

This kind of init code is everywhere, I can always see the potential repetition here and there. We need to have a policy on these kind of code and apply them consistently. Some of them are tangled and less easy to extract. This one looks not so much tangled though.

The old code likes to use some lazy-init-like style. Upon calling a method to get an attribute, it will check if a certain condition is met, then do init. But the condition isn't always the same as whether it's inited. And the old code like to use these lazy-ish getter method to do the init without a init-like name.

I imagine that the policy to apply could be:

  1. extract these checking logic into a dedicated init private method with an init-like name;
  2. for a getter, just make it like a plain getter or a standard lazy-init getter.

This is consistent with what you are doing.

@utensil utensil merged commit a29f146 into pygae:master Nov 27, 2019
6 checks passed
6 checks passed
LGTM analysis: Python No new or fixed alerts
Details
Travis CI - Pull Request Build Passed
Details
codebeat no reportable quality changes
Details
codecov/patch 80% of diff hit (target 67.24%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +12.75% compared to 6b8055b
Details
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
@eric-wieser eric-wieser deleted the eric-wieser:compute-grads-once branch Nov 27, 2019
@utensil utensil mentioned this pull request Nov 29, 2019
0 of 5 tasks complete
@utensil utensil added this to the 0.4.5 milestone Nov 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.