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

Add extra check if both values are NA in bulk_upsert #53

Merged
merged 7 commits into from
Feb 29, 2024

Conversation

meksor
Copy link
Contributor

@meksor meksor commented Feb 29, 2024

Fixes #50 's pyarrrow compatibility problem.

@@ -391,7 +391,10 @@ def bulk_upsert_chunk(self, df: pd.DataFrame) -> None:
for col in self.model_class.updateable_columns:
updated_col = col + self.merge_suffix
if updated_col in df.columns:
cond.append(df[col] != df[updated_col])
df[updated_col] = df[updated_col].astype(df[col].dtype)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a line explaining why this is necessary? And a test (in a GitHub Action that explicitly installs pyarrow) to guard against regression?

@glatterf42
Copy link
Member

Do we intend to release 0.7.1 immediately after this fix so that IAMconsortium/pyam#820 can move forward?

Copy link
Member

@glatterf42 glatterf42 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, provided the tests pass. But I'm not sure I like test names like "test / test (3.11, false) (pull_request) ". Doesn't seem as descriptive as it could be, but I don't have a tangible better solution of the top of my head.

@meksor
Copy link
Contributor Author

meksor commented Feb 29, 2024

Alright I added a test matrix entry for pyarrow and python 3.12. Also added some comments.
And yes, I would release it after merging this.

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Nice!

@meksor meksor merged commit c85d987 into main Feb 29, 2024
6 checks passed
@meksor meksor deleted the bugfix/upsert-to-none branch February 29, 2024 14:12
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.

Version 0.7.0 breaks pyam test
3 participants