Skip to content
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

Feature/return df #76

Closed
wants to merge 8 commits into from
Closed

Feature/return df #76

wants to merge 8 commits into from

Conversation

kevindougherty-noaa
Copy link
Collaborator

@kevindougherty-noaa kevindougherty-noaa commented Oct 5, 2021

Addresses #75

@kevindougherty-noaa kevindougherty-noaa marked this pull request as ready for review October 5, 2021 18:07
@kevindougherty-noaa
Copy link
Collaborator Author

@CoryMartin-NOAA still need to go through the other files for pycodestyle to pass, however diags.py passes which is the only file I changed.

@CoryMartin-NOAA
Copy link
Contributor

@kevindougherty-noaa can you explain to @ADCollard and me why this PR is needed? I'm sure it is a method vs attribute thing but it's not clear to me at the moment.

@kevindougherty-noaa
Copy link
Collaborator Author

@CoryMartin-NOAA @ADCollard Sure thing. In our regional DA meeting last week, Ting and Shun mentioned interest in being able to have the entire dataframe returned. I could be mistaken and maybe they meant returning the dataframe after being indexed.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

See comment.

pyGSI/diags.py Outdated
Comment on lines 646 to 649
def return_df(self):
"""Returns working dataframe."""
return self.data_df

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you add this method in Class GSIdiag since that is the class from which all other inherit from?

@CoryMartin-NOAA
Copy link
Contributor

@kevindougherty-noaa yes I remember that, so I guess you're saying this is so one can have numpy arrays if they want, and a Pandas dataframe through the .return_df() method? I thought the issue you linked would basically imply that the read method would have an argument like read_data(return_df=True) and then:

if return_df:
    return self.df
else:
   return data

or whatever the above pseudo code should be

@kevindougherty-noaa
Copy link
Collaborator Author

@CoryMartin-NOAA you are right, that is what should be done. I will fix.

pyGSI/diags.py Outdated
}

return data
if return_df:
Copy link
Contributor

Choose a reason for hiding this comment

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

@kevindougherty-noaa is this indentation correct?

'monitored': v_monitored}

return u, v
if return_df:
Copy link
Contributor

Choose a reason for hiding this comment

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

so for non data frame, it returns a dict of dicts, for u,v. For the data frame case, are both u and v inside the data frame?

Copy link
Collaborator Author

@kevindougherty-noaa kevindougherty-noaa Oct 5, 2021

Choose a reason for hiding this comment

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

Yes, both u and v are in the dataframe. All the variables will be included.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Just a quick glance.
See if you notice these as well.

pyGSI/diags.py Outdated Show resolved Hide resolved
pyGSI/diags.py Outdated Show resolved Hide resolved
kevindougherty-noaa and others added 3 commits October 6, 2021 09:58
@kevindougherty-noaa
Copy link
Collaborator Author

Based on conversations with @CoryMartin-NOAA and @aerorahul, I am going to close this PR and develop diags.py to only return dataframes rather than specific variables. It should cut out a good amount of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants