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

DX enhancements #110

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

DX enhancements #110

wants to merge 4 commits into from

Conversation

RileyMShea
Copy link
Contributor

@RileyMShea RileyMShea commented Feb 28, 2021

Made some minor changes towards enhancing dx and intellisense/autocomplete for end-user.

- added section to silence Deprecation warning spam from numpy/numba.
- Began Refactoring of Enums b/c to improve intellisense,
editor autocomplete for end-user.  Numba seems to support this:
https://numba.pydata.org/numba-doc/dev/reference/pysupported.html#enum

- Also started refactoring named_tuple function-based creation
with class-based creation, because it provides better intellisense,
autocomplete.

Would like to do more in this regard if @polakowo approves of this.
@codecov
Copy link

codecov bot commented Feb 28, 2021

Codecov Report

Merging #110 (92ce571) into master (4152f22) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   90.41%   90.45%   +0.03%     
==========================================
  Files          60       60              
  Lines        9006     9027      +21     
  Branches     2040     2041       +1     
==========================================
+ Hits         8143     8165      +22     
  Misses        421      421              
+ Partials      442      441       -1     
Impacted Files Coverage Δ
vectorbt/portfolio/enums.py 100.00% <100.00%> (ø)
vectorbt/signals/enums.py 100.00% <100.00%> (ø)
vectorbt/indicators/factory.py 88.31% <0.00%> (+0.13%) ⬆️

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 4152f22...92ce571. Read the comment docs.

@polakowo
Copy link
Owner

Good start, native enum classes are indeed much more intuitive than named tuples, just make sure to adapt https://github.com/polakowo/vectorbt/blob/master/vectorbt/utils/enum.py, also is_namedtuple in https://github.com/polakowo/vectorbt/blob/master/vectorbt/utils/checks.py is currently being used for checking enums, and look for every bit of code that accesses _fields attribute, maybe create a tiny function that returns the member names. And we could define enums values with auto() starting from 0 or at least use the unique decorator to rule out duplicate values.

@RileyMShea
Copy link
Contributor Author

awesome, will try to continue with this and your suggestions when I have free time.

@RileyMShea
Copy link
Contributor Author

RileyMShea commented Mar 1, 2021

@polakowo One issue is that using asterisk arg expand like *SimulationContext._fields, seems to break intellisense/autocomplete when defining the nametuples.
image

Without the * autocomplete works good:
image

since NameTuples don't have the ability to inherit fields when created as a class, and * expansion breaks intellisense for function-based creation of namedtuple,

it seems not possible to both inherit fields and keep editor autocomplete/intellisense.

Current Options seem to be:

  • Keep current implementation in master
  • Rewrite nametuples, accepting downside that inherited fields will have to be listed explicitly(duplicated) but will gain intellisense.
  • Use composition over inheritence to embed namedtuples inside other nametuples fields, in-place of the current inheriting field names via * expansion:

image

  • Other?

TLDR: It seems to me that refactoring into NameTuples utilizing composition (nametuple embedding) would be ideal solution but would probably require the most work since I'm guessing it would probably need updating where many parts of the codebase expect values. I might be overlooking something where that's not even possible though.

Wondering which of these options you think I should pursue?(if any)

@polakowo
Copy link
Owner

polakowo commented Mar 1, 2021

@RileyMShea hmm interesting observation, I just tried to come up with some hack but was unable to find any.

Your composition approach makes a lot of sense given that all simulation functions follow a clear hierarchy: prep_func_nb > group_prep_func_nb | row_prep_func_nb > segment_prep_func_nb -> order_func_nb, so this can be well reflected through embedding. You would need to update tuple creation in simulate_nb and simulate_row_wise_nb and also search for usage of simc, gc, rc, sc, oc, and mixes of them (I guess there is only one sc_oc, that is, it can take both the sc and oc) in the codebase and examples/documentation (there aren't many though).

But be aware that usage of contexts is poorly tested, so making mistakes would still probably give you green light in pytest. If you decide to pursue all these changes, I could then follow up with writing unit tests.

And I really appreciate your time and effort. It's time to diversify this codebase for the good :)

I'm going to release 0.17 in a couple of days with refactored and better-documented indicator factory and some cross-validation functions, but you're safe to continue working on the current master, I won't touch anything related to enums and portfolio.

@RileyMShea
Copy link
Contributor Author

@polakowo Okay awesome, I will try to work towards those changes u mention. I am excited for 0.17!

@RileyMShea
Copy link
Contributor Author

@polakowo The hours at my main job have increased to where I currently don't have time to allot to this PR. I would still like to complete it, but it may be a while before I can find time to work on it.

@polakowo
Copy link
Owner

polakowo commented Mar 7, 2021

@RileyMShea it's fine, you can jump in anytime, we're not in any hurry :) Wish you a stress-free holiday!

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