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

ENH: Function to detect sign of a number #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dthanasekaran
Copy link

Description

added a feature to deduce sign of a float

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

@@ -0,0 +1,23 @@
import pytest
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you see in the test suite here that things are failing, saying numpy cannot be found?

You might wonder why it works while using pcds_conda but not here.
That's because each of our packages have their own set of requirements. We first test them in isolation - not as part of the conda environment we use on a daily basis.

You can fix this by adding numpy as a dependency. We specify dependencies in a couple spots for the package manager pip (ignoring the conda builds for now):

  1. For "normal" requirements, required to run a package as a user, these would go in requirements.txt.
  2. For requirements that are used only in the test suite, those go in dev-requirements.txt ("dev" for "development", of course)

Since numpy is only used in your test suite in this PR, it should go in dev-requirements.txt. If you open that file, you'll see that pytest is listed there currently, because it's what we use to run the test suite.

@@ -0,0 +1,23 @@
import pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

There's one more thing failing in the "pre-commit" check.

Did you use pre-commit when committing your code? It automatically sorts imports and tells you about style issues before it gets to GitHub.

The output here:

isort....................................................................Failed
- hook id: isort
- files were modified by this hook

Fixing /home/runner/work/pcds-ci-test-repo-python/pcds-ci-test-repo-python/project_name/tests/test_sign.py

pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/project_name/tests/test_sign.py b/project_name/tests/test_sign.py
index 6fc37e4..14c7ffc 100644
--- a/project_name/tests/test_sign.py
+++ b/project_name/tests/test_sign.py
@@ -1,5 +1,5 @@
-import pytest
 import numpy as np
+import pytest

Tells you that isort would have put numpy before pytest, because they are sorted alphabetically by convention.

To fix it, you can do as it says above:

$ pre-commit install
$ pre-commit run --all-files

Or manually fix based on what it says, of course. I'm lazy and prefer to have pre-commit take care of it for me, though!

@klauer
Copy link
Contributor

klauer commented Feb 13, 2023

Thanks for the PR, @dthanasekaran!

In case you're interested, I detailed the reasons the above tests are failing and how you could resolve them.
Don't feel obligated to fix it all up, but if you want to I'm happy to walk you through it.

tangkong
tangkong previously approved these changes Sep 27, 2023
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.

4 participants