Ineffectual PR to add my commits messages back #562
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Warning: ChainedAssignmentError: A value is trying to be set on a copy of a DataFrame or Series through chained assignment using an inplace method. When using the Copy-on-Write mode, such inplace method never works to update the original DataFrame or Series, because the intermediate object on which we are setting values always behaves as a copy. I just to just not use "inplace" here and the warning does not happen.
Previously this warning happened:
FutureWarning: Downcasting behavior in `replace` is deprecated and will be removed in a future version. To retain the old behavior, explicitly call `result.infer_objects(copy=False)`. To opt-in to the future behavior, set `pd.set_option('future.no_silent_downcasting', True)`
df.replace("", np.nan, inplace=True)
setting the option stops Pandas from even trying to downcast (so it no longer has anything to warn about). I then force the downcast explicitly with infer_objects to ensure that the behaviour remains what it was, but not sure this is what I want here.
df.infer_objects(copy=False) does not work in a platform independent manner; and I dont even know if I need it
As @gouttegd suggested: > I would rather put that somewhere when the library is initialized, so that we can be sure the option is enabled everywhere at all times and not only when the code path goes through this particular function. I moved that code to __init__.py hopeing that is the right place for it.
As per @gouttegd suggestion here https://github.com/mapping-commons/sssom-py/pull/561/files#r1835130707, the codeblock has been refactored and simplified.
To ensure the old behaviour is retained, @gouttegd convinced me to not be lazy and actually add the "df.infer_objects" functionality when replacing empty values with nan's. Should github still exist when you read this, you can find his feedback here: https://github.com/mapping-commons/sssom-py/pull/561/files#r1835148927 As he questions, it is not clear this is _strictly needed_, but its probably safer to so if we dont know if its needed.
The rationale is to not force this global option on all users of the library. The main point for the fix is so that sssom-py's CLI print to stdout functionality is not compromised by this output!
As explained by @gouttegd in #561 (comment), we do not actually benefit from the small performance gain enabled by copy=False here, so we decided to drop it (which means a considerable simplification of the code as well)
gouttegd
approved these changes
Nov 9, 2024
|
Better not make things even worse than I already did.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.