error resolution in sas file #43326
Closed
error resolution in sas file #43326
Conversation
This file contains hidden or 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
pranava1709
commented
Aug 31, 2021
pranava1709
commented
- closes Reading a SAS file with 0 rows gives None #18198
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- SAS file is now giving empty dataframe. Not NONE.
|
hi @pranava1709 seems like a fat finger or mistaken PR. You can't really just add random python script files that don't have any impact on the codebase. |
|
The python script that I have added solves one of the issues. I have also
tested it on my local computer. You can cross-check the issue.
…On Tue, Aug 31, 2021 at 10:28 PM JHM Darbyshire ***@***.***> wrote:
hi @pranava1709 <https://github.com/Pranava1709> seems like a fat finger
or mistaken PR. You can't really just add random python script files that
don't have any impact on the codebase.
Please re-open and re-commit if you have code change contributions to make
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43326 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOP7GWZAMUQPOKF6W5QSDCTT7UC4RANCNFSM5DD5CVMQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
This is outside of anything Pandas uses. How does that solve any issues? edit: please read the contributing guidelines |
|
My code has used only one library which is pandas. The issue(#43326) was
that the SAS file with 0 rows was giving NONE, while CSV was giving an
empty data frame. With this script, SAS file is also giving empty data
frame instead of none.
…On Tue, Aug 31, 2021 at 10:53 PM Patrick Hoefler ***@***.***> wrote:
This is outside of anything Pandas uses. How does that solve any issues?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43326 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOP7GW7MV6V3VSIGNUTULWLT7UFZDANCNFSM5DD5CVMQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
But how does this fix the pandas function read_sas? |
|
It does not fix read_sas. It fixes the issue that was coming in the output.
According to the issue, there is no mention that the issue is in read_sas.
I solved this issue keeping the problem in mind that it was not giving an
empty data frame with 0 rows.
So I took a file, removed the rows and checked it and got the required
output.
…On Tue, Aug 31, 2021 at 11:04 PM Patrick Hoefler ***@***.***> wrote:
But how does this fix the pandas function read_sas?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43326 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOP7GW27APJCCYRYC5LLNLDT7UHCVANCNFSM5DD5CVMQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
We would want read_sas to return the correct result. Everything Else does Not help out users. Please read the contributing guidelines |
|
It is returning. It's just I had a random dataset, in which I removed the
rows. If you pass an original file with 0 rows read_sas will give the same
output. The extra part is the preprocessing of data that's it as the file
was not empty.
…On Tue, Aug 31, 2021 at 11:12 PM Patrick Hoefler ***@***.***> wrote:
We would want read_sas to return the correct result. Everything Else does
Not help out users. Please read the contributing guidelines
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43326 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOP7GW6DP3U3IPPBOBW6W6TT7UH7RANCNFSM5DD5CVMQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Then the function already is correct, so no need to add anything except a Test case |
|
This was an issue so I took it up.
…On Tue, Aug 31, 2021 at 11:18 PM Patrick Hoefler ***@***.***> wrote:
Then the function already is correct, so no need to add anything except a
Test case
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43326 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOP7GWZLW3SULQJUSTERHBDT7UIXRANCNFSM5DD5CVMQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
If an issue is already fixed we add a test to close and avoid regressions. |
|
So what do I do now? I took this issue for my internship. They wanted to
fix one of the issues and send the pull request to them.
…On Tue, Aug 31, 2021 at 11:21 PM Patrick Hoefler ***@***.***> wrote:
If an issue is already fixed we add a test to close and avoid regressions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43326 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOP7GWY6KK5F7LEIJF6TQFTT7UJBVANCNFSM5DD5CVMQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Add a test checking that an empty sas file is read as an empty dataframe. |
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.