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

Add sqrt, log, sin, cos and tan transform primitives #1948

Merged
merged 9 commits into from Mar 15, 2022

Conversation

tvdboom
Copy link
Contributor

@tvdboom tvdboom commented Mar 11, 2022

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2022

CLA assistant check
All committers have signed the CLA.

@tvdboom
Copy link
Contributor Author

tvdboom commented Mar 11, 2022

I couldn't find a test where all primitives are systematically tested. Do I need to add a separate test for each or how should it be done?

@thehomebrewnerd
Copy link
Contributor

I couldn't find a test where all primitives are systematically tested. Do I need to add a separate test for each or how should it be done?

@tvdboom First, thanks for the contribution!

Second, the transform primitives should all get tested in test_transform_features.py in the test_init_and_name test:

That test should make sure that a feature is created using the primitive and that it calculates without error. That test does not actually test that the calculation is being performed correctly, but since you are using standard numpy functions, I don't think we need to worry about testing the results for correctness in this case, outside of the docstring tests.

@gsheni gsheni changed the title added sqrt, log, sin, cos and tan primitives Add sqrt, log, sin, cos and tan transform primitives Mar 11, 2022
@tvdboom
Copy link
Contributor Author

tvdboom commented Mar 11, 2022

@thehomebrewnerd So if I understood correctly, since test_init_and_name fetches all transform primitives, the new primitives are already being tested, so I don't need to add anything else for that. What about the documentation? I added the primitives to the API reference. Does it need to be added somewhere else?

@tvdboom tvdboom mentioned this pull request Mar 11, 2022
3 tasks
@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #1948 (2ef3057) into main (9d59bed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1948   +/-   ##
=======================================
  Coverage   98.99%   98.99%           
=======================================
  Files         146      146           
  Lines       16438    16478   +40     
=======================================
+ Hits        16273    16313   +40     
  Misses        165      165           
Impacted Files Coverage Δ
...retools/primitives/standard/transform_primitive.py 100.00% <100.00%> (ø)

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 9d59bed...2ef3057. Read the comment docs.

@thehomebrewnerd
Copy link
Contributor

@thehomebrewnerd So if I understood correctly, since test_init_and_name fetches all transform primitives, the new primitives are already being tested, so I don't need to add anything else for that. What about the documentation? I added the primitives to the API reference. Does it need to be added somewhere else?

I think adding these to the API reference should be sufficient.

@thehomebrewnerd
Copy link
Contributor

@tvdboom Did you by chance enable github actions to run on your fork of the repo? I am getting dozens of notifications that some of our automated workflows are running and failing on your fork. Unfortunately your forked repo doesn't have the secrets setup for these workflows to succeed and they will continue to fail and send the notifications of the failure.

If actions are enabled, could you please disable them in the settings page?

image

On another note, I'll take a final look through your PR today, but I think we should be close to approving and merging.

Copy link
Contributor

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks again for the contribution!

@thehomebrewnerd thehomebrewnerd merged commit 61876c8 into alteryx:main Mar 15, 2022
This was referenced Mar 15, 2022
@thehomebrewnerd
Copy link
Contributor

@tvdboom Can you please review my message above about the github actions? I am continuing to get dozens of failed action run notices in my email daily about actions failing on your fork of the repo? So far I have not found a way to disable these notifications since they are coming from a repo you own.

Can you please disable actions on your fork of this repo by going to Settings -> Actions -> General in GitHub?

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.

Add Sine, Cosine, Tangent, SquareRoot, Log primitives
3 participants