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

Fix issues with auto_entityset #21

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix issues with auto_entityset #21

wants to merge 10 commits into from

Conversation

thehomebrewnerd
Copy link
Contributor

This PR fixes two issues that were identified when trying to replicate the error described in Issue #19

The first change addresses a problem that resulted by trying to do an == comparison between nan values.

The second change fixes an issue that can happen causing a column that is needed in other relationships to be dropped from an entity when make_indexes is executed.

Two additional tests were added to cover these scenarios.

Also, note, while these issues were discovered when testing for Issue #19, that specific error was not reproduced and it is unknown whether these changes will resolve that issue or not.

@thehomebrewnerd thehomebrewnerd requested a review from rwedge March 30, 2020 16:03
@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@19acb46). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #21   +/-   ##
=========================================
  Coverage          ?   94.74%           
=========================================
  Files             ?       10           
  Lines             ?      970           
  Branches          ?        0           
=========================================
  Hits              ?      919           
  Misses            ?       51           
  Partials          ?        0           
Impacted Files Coverage Δ
autonormalize/dfd.py 98.10% <100.00%> (ø)
autonormalize/normalize.py 98.19% <100.00%> (ø)
autonormalize/tests/test_dfd.py 91.17% <100.00%> (ø)
autonormalize/tests/test_normalize.py 100.00% <100.00%> (ø)
autonormalize/__init__.py 100.00% <0.00%> (ø)
autonormalize/pyspy2.py 0.00% <0.00%> (ø)
autonormalize/classes.py 95.07% <0.00%> (ø)
autonormalize/autonormalize.py 70.45% <0.00%> (ø)
autonormalize/tests/test_example.py 100.00% <0.00%> (ø)
... and 4 more

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 19acb46...4fa5693. Read the comment docs.

Base automatically changed from master to main February 19, 2021 19:30
@CLAassistant
Copy link

CLAassistant commented Jan 10, 2022

CLA assistant check
All committers have signed the CLA.

masks.add_mask(attr, row[attr], m)
if mask is None:
mask = m
else:
mask = mask & m
options = df[mask]
_, unique_counts = numpy.unique(options[rhs].to_numpy(), return_counts=True)

# _, unique_counts = np.unique(options[rhs].to_numpy(), return_counts=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this line be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, removed this and another temporary test that I added trying to replicate the problem that was present in #19.

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