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

Dont replicate nans in time #226

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Dont replicate nans in time #226

merged 2 commits into from
Oct 21, 2024

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Oct 21, 2024

This is a very small fix to the function replicate_nan_values which replicates NaN values in columns that are common between two dataframes from one to another. For some reason this does not work for the time column when it is of type DateTime64DType. However, I can't think of a good reason we need to expect to replicate nans in the time column (this functionality is for marking missing turbine data). So simply omitting this column avoids the issue.

@paulf81 paulf81 added the bug Something isn't working label Oct 21, 2024
@paulf81 paulf81 requested a review from misi9170 October 21, 2024 20:13
@paulf81 paulf81 self-assigned this Oct 21, 2024
Copy link
Collaborator

@misi9170 misi9170 left a comment

Choose a reason for hiding this comment

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

Only one minor syntax suggestion, but this looks fine to me

@@ -49,6 +49,10 @@ def replicate_nan_values(
# Identify common columns between df_1 and df_2
common_columns = df_1.columns.intersection(df_2.columns)

# Remove the time column from the common columns if included
if "time" in common_columns:
common_columns = common_columns.drop("time")
Copy link
Collaborator

@misi9170 misi9170 Oct 21, 2024

Choose a reason for hiding this comment

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

Might be cleaner to use pandas' built-in check for whether the column exists to avoid the if statement?

# Remove the time columns from the common_columns if included
common_columns.drop("time", errors="ignore")

Either way is fine though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion, using that syntax, thanks!

@paulf81 paulf81 merged commit 44e9bde into NREL:develop Oct 21, 2024
3 checks passed
@paulf81 paulf81 deleted the bug_fix/fit_time branch October 21, 2024 20:38
@paulf81 paulf81 restored the bug_fix/fit_time branch October 21, 2024 20:41
@paulf81 paulf81 mentioned this pull request Oct 21, 2024
@misi9170 misi9170 mentioned this pull request Oct 22, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants