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

Handles NaN's #30

Merged
merged 10 commits into from
Oct 12, 2021
Merged

Handles NaN's #30

merged 10 commits into from
Oct 12, 2021

Conversation

fealho
Copy link
Member

@fealho fealho commented Sep 8, 2021

This PR has two purposes:

  1. Resolves ValueError: The parameter loc has invalid values SDV#353. In that issue the user was passing NaN`s to SDV, and in turn SDV was passing them to DeepEcho, which was unable to handle them. This PR addresses this.

  2. Update dependency versions. Since no one has worked on DeepEcho in a while, the latest versions of some libraries became incompatible. This PR caps those dependencies to working versions.

Resolves #35.

@fealho fealho marked this pull request as ready for review September 8, 2021 20:43
@fealho fealho requested a review from csala September 8, 2021 20:44
Copy link
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

This looks fine to go!
Maybe it would be interesting to open an issue here, in DeepEcho, explaining the lack of support for np.nan and pointing at it from this PR, just to make everything consistent and keep track of the change.

@@ -210,17 +210,23 @@ def _data_to_tensor(self, data):

elif props['type'] in ['continuous', 'timestamp']:
mu_idx, sigma_idx, missing_idx = props['indices']
x[mu_idx] = 0.0 if (data[key][i] is None or props['std'] == 0) else (
data[key][i] - props['mu']) / props['std']
if pd.isnull(data[key][i]) or props['std'] == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the change from one-liner to if / else block!
It improves readability a lot, in this case.

@csala csala requested a review from a team September 9, 2021 15:51
@csala csala requested review from katxiao and removed request for a team October 8, 2021 18:08
@fealho fealho merged commit f515be3 into master Oct 12, 2021
@fealho fealho deleted the sdv-issue-353-handle-nan branch October 12, 2021 14:26
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.

Handling NaN's ValueError: The parameter loc has invalid values
3 participants