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

Changed to intuitive casting, removed dtype tests from basic test class #598

Merged
merged 8 commits into from
Jul 1, 2020

Conversation

coquelin77
Copy link
Member

@coquelin77 coquelin77 commented Jun 17, 2020

Description

The dtype comparison was removed as the basic tests works with numpy, which does safe casting instead of intuitive casting. other changes were adjustments for types

Issue/s resolved:

Changes proposed:

  • change from safe casting to intuitive casting (this come with the possibility of loss during the conversion)

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Due Diligence

  • All split configurations tested
  • Multiple dtypes tested in relevant functions
  • Documentation updated (if needed)
  • Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

no

The dtype comparison was removed as the basic tests works with numpy, which does safe casting instead of intuitive casting.
@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #598 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #598      +/-   ##
==========================================
- Coverage   97.48%   97.46%   -0.02%     
==========================================
  Files          77       77              
  Lines       15429    15396      -33     
==========================================
- Hits        15041    15006      -35     
- Misses        388      390       +2     
Impacted Files Coverage Δ
heat/core/tests/test_exponential.py 100.00% <100.00%> (ø)
heat/core/tests/test_rounding.py 100.00% <100.00%> (ø)
heat/core/tests/test_suites/basic_test.py 90.90% <100.00%> (-0.44%) ⬇️
heat/core/tests/test_suites/test_basic_test.py 100.00% <100.00%> (ø)
heat/core/tests/test_trigonometrics.py 100.00% <100.00%> (ø)
heat/core/tests/test_types.py 100.00% <100.00%> (ø)
heat/core/types.py 94.84% <100.00%> (+0.51%) ⬆️
heat/spatial/tests/test_distances.py 100.00% <100.00%> (ø)
heat/spatial/distance.py 95.74% <0.00%> (-1.60%) ⬇️

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 afd1968...a011794. Read the comment docs.

Copy link
Contributor

@ClaudiaComito ClaudiaComito 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 to me! Does the promote_types() documentation need an update?

heat/core/tests/test_suites/basic_test.py Outdated Show resolved Hide resolved
heat/core/types.py Outdated Show resolved Hide resolved
@ClaudiaComito ClaudiaComito added this to the 2-week sprint milestone Jun 30, 2020
Copy link
Member

@Markus-Goetz Markus-Goetz left a comment

Choose a reason for hiding this comment

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

  • Changelog needs an update

Claudia's comment:

  • Update to the docstring with double quotation marks

@coquelin77
Copy link
Member Author

* Changelog needs an update

Claudia's comment:

* Update to the docstring with double quotation marks

docstrings have double quotation marks now, changelog is updated

@Markus-Goetz Markus-Goetz merged commit b47f003 into master Jul 1, 2020
@Markus-Goetz Markus-Goetz deleted the features/597-intuitive-casting branch July 1, 2020 14:06
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.

3 participants