-
Notifications
You must be signed in to change notification settings - Fork 110
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
SNOW-1518378 - Provide a numpy compatibility mapping to np.full_like #2499
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have a couple of small unresolved comments
…ew/numpy.full_like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some nits, but great work overall!
@@ -80,6 +80,9 @@ | |||
- Added support for applying Snowpark Python function `snowflake_cortex_summarize`. | |||
- Added support for `DataFrame.attrs` and `Series.attrs`. | |||
- Added support for `DataFrame.style`. | |||
- Added support for `Index.to_numpy`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are probably merge artifacts?
return NotImplemented | ||
if not order == "K": | ||
return NotImplemented | ||
if dtype is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we support dtype here? It just overrides the datatype provided by a right?
result_shape = (result_shape,) | ||
if result_shape is None: | ||
result_shape = a.shape | ||
if len(result_shape) == 2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check if result_shape
is list-like? otherwise something like result_shape = 'ab' would enter this conditional?
result_shape = a.shape | ||
if len(result_shape) == 2: | ||
height, width = result_shape # type: ignore | ||
return pd.DataFrame(fill_value, index=range(height), columns=range(width)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to set dtype here? Also if a DataFrame contains multiple dtypes, are the returned object's columns supposed to be the same type as each of the columns mapped positionally?
height, width = result_shape # type: ignore | ||
return pd.DataFrame(fill_value, index=range(height), columns=range(width)) | ||
if len(result_shape) == 1: | ||
return pd.Series(fill_value, index=range(result_shape[0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dtype?
with SqlCounter(query_count=2): | ||
snow_result = np.full_like(snow_df, 1234) | ||
pandas_result = np.full_like(pandas_df, 1234) | ||
assert_array_equal(np.array(snow_result), np.array(pandas_result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be checking if the dataframes are equal?
SNOW-1518378 - Provide a numpy compatibility mapping to np.full_like
This provides a numpy compat mapping to np.full_like; which fills a dataframe/series with a fill value the same shape as the input.
checklist: