Skip to content

Conversation

@chris-langfield
Copy link
Collaborator

DataFrame.update() takes care of what we were trying to do previously, which was including new data but setting old data to NaN.
https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.update.html

See #635 for the original issue.

Also, by casting the series to dtype object, when the same column exists in both the original DF, and the new DF it is being updated from, the dtype will default to that of the original DF. This fixes the second issue mentioned in #635.
https://stackoverflow.com/a/47145613/9584059

@chris-langfield chris-langfield self-assigned this Jun 22, 2022
@chris-langfield chris-langfield added bug Something isn't working cleanup labels Jun 22, 2022
@chris-langfield chris-langfield requested a review from j-c-c June 22, 2022 19:48
@chris-langfield chris-langfield linked an issue Jun 22, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Merging #648 (0111efe) into develop (1bff8d3) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #648   +/-   ##
========================================
  Coverage    86.49%   86.49%           
========================================
  Files          109      109           
  Lines         8286     8286           
========================================
  Hits          7167     7167           
  Misses        1119     1119           
Impacted Files Coverage Δ
src/aspire/source/image.py 96.73% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Collaborator

@j-c-c j-c-c 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!

)
else:
self._metadata[metadata_field] = series
self._metadata.update(series.astype(object))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well that's convenient!

@chris-langfield chris-langfield marked this pull request as ready for review June 30, 2022 19:46
@chris-langfield chris-langfield requested a review from janden as a code owner June 30, 2022 19:46
@garrettwrong garrettwrong self-requested a review July 8, 2022 14:14
Copy link
Collaborator

@garrettwrong garrettwrong 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. Thanks Chris. Lets go ahead and merge this one too.

@chris-langfield chris-langfield merged commit 4e5fec5 into develop Jul 21, 2022
@chris-langfield chris-langfield deleted the set_metadata_635 branch July 21, 2022 17:44
j-c-c pushed a commit that referenced this pull request Jul 22, 2022
* fix metadata bug

* put space back in

* format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ImageSource.set_metadata() bug

5 participants