8000 Added type to DataFrame.rename mapper by Devwulf · Pull Request #347 · javascriptdata/danfojs · GitHub
[go: up one dir, main page]
Skip to content

Added type to DataFrame.rename mapper #347

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 14, 2022
Merged

Added type to DataFrame.rename mapper #347

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/danfojs-base/core/frame.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2754,14 +2754,18 @@ export default class DataFrame extends NDframe implements DataFrameInterface {
*
*/
rename(
mapper: any,
mapper: {
[index: string | number]: string | number
},
options?: {
axis?: 0 | 1
inplace?: boolean
}
): DataFrame
rename(
mapper: any,
rename(
mapper: {
[index: string | number]: string | number
},
options?: {
axis?: 0 | 1
inplace?: boolean
Expand All @@ -2777,8 +2781,9 @@ export default class DataFrame extends NDframe implements DataFrameInterface {
const colsAdded: string[] = [];
const newColumns = this.columns.map(col => {
if (mapper[col] !== undefined) {
colsAdded.push(mapper[col]);
return mapper[col]
const newCol = `${mapper[col]}`;
Copy link
Member

Choose a reason for hiding this comment

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

Hey @Devwulf I'm not sure why you did this, can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Devwulf I'm not sure why you did this, can you explain?

So with the new mapper type, you can type in either a string or a number as the value in a mapping pair. Since the dataframe columns array only accept strings, line 2784 basically converts each mapped value as strings. This is so they can be pushed into the colsAdded array and so that newColumns stay as a string[] instead of being a (string | number)[] array.

Perhaps this function can also be separated into two with the only difference being the axis value they take in, but you might as well start doing this to all functions that take in an axis option, which might complicate things. This is what I'm thinking about:

rename(
    mapper: {
        [index: string]: string
    }, 
    options?: {
        axis?: 1, // this function is returned if axis is 1
        inplace?: boolean
    }
): DataFrame | void {
    // Rename the columns
}

rename(
    mapper: {
        [index: string | number]: string | number
    }, 
    options?: {
        axis?: 0, // this function is returned if axis is 0
        inplace?: boolean
    }
): DataFrame | void {
    // Rename the row indexes
}

It might be convenient to do something similar as well for the inplace option, so that the function returns the proper values (either void or DataFrame, not both) depending on the inplace option, since I find myself always casting to DataFrame every time inplace is false.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @Devwulf I'm not sure why you did this, can you explain?

So with the new mapper type, you can type in either a string or a number as the value in a mapping pair. Since the dataframe columns array only accept strings, line 2784 basically converts each mapped value as strings. This is so they can be pushed into the colsAdded array and so that newColumns stay as a string[] instead of being a (string | number)[] array.

Perhaps this function can also be separated into two with the only difference being the axis value they take in, but you might as well start doing this to all functions that take in an axis option, which might complicate things. This is what I'm thinking about:

rename(
    mapper: {
        [index: string]: string
    }, 
    options?: {
        axis?: 1, // this function is returned if axis is 1
        inplace?: boolean
    }
): DataFrame | void {
    // Rename the columns
}

rename(
    mapper: {
        [index: string | number]: string | number
    }, 
    options?: {
        axis?: 0, // this function is returned if axis is 0
        inplace?: boolean
    }
): DataFrame | void {
    // Rename the row indexes
}

It might be convenient to do something similar as well for the inplace option, so that the function returns the proper values (either void or DataFrame, not both) depending on the inplace option, since I find myself always casting to DataFrame every time inplace is false.

Oh my bad, I see why now. Thanks for taking out time to explain, and yes you're correct, columns only take in string values.

colsAdded.push(newCol);
return newCol;
} else {
return col
}
Expand Down
0