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

Add some doc strings #342

Merged
merged 2 commits into from
Feb 23, 2023
Merged

Conversation

kcpevey
Copy link
Contributor

@kcpevey kcpevey commented Feb 22, 2023

  • Add doc strings to new methods
  • Add examples as doc test

Also while waiting for tests to run I added a few "close figs"

@kcpevey
Copy link
Contributor Author

kcpevey commented Feb 22, 2023

There were some other doc strings that I could have added but I was concerned about working on top of @MarcoGorelli ad @CommonClimate on changing code.

@CommonClimate
Copy link
Collaborator

Good thinking @kcpevey . Is this ready for review, then, or will you open another PR once we're done with code edits, so you can put in the other docstrings?

Copy link
Collaborator

@CommonClimate CommonClimate left a comment

Choose a reason for hiding this comment

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

Looks good! I might just change "pyleo_series" to "ts" in the load_dataset() docstring, as we typically use that moniker for a Series, the same way pandas uses "df" for dataframe, even though anything is allowed.

@CommonClimate
Copy link
Collaborator

CommonClimate commented Feb 23, 2023

Also, just saw the docstring for sel(), which is nice and comprehensive. In terms of the screen output, though, it won't look like this anymore:
{'log': ({0: 'clean_ts', 'applied': True, 'verbose': False}, {2: 'clean_ts', 'applied': True, 'verbose': False})}

It should be:

{'log': ({0: 'dropna', 'applied': True, 'verbose': True}, {1: 'sort_ts', 'direction': 'ascending'})}

Sorry to change this on you. So I guess this way of making docstrings does not take changes in the codebase into account. That is an argument for using ipython directives...

@kcpevey
Copy link
Contributor Author

kcpevey commented Feb 23, 2023

I might just change "pyleo_series" to "ts" in the load_dataset() docstring, as we typically use that moniker for a Series, the same way pandas uses "df" for dataframe, even though anything is allowed.

I can make that change to this PR.

Screen output for sel. That is the section I'm avoiding on working on so I will wait until the dust settles and open another PR.

@CommonClimate
Copy link
Collaborator

OK, I like that plan. Re: docstrings, the SeriesResampler will need a lot of explanation. Honestly, I still don't understand exactly how it works, and I doubt our users will. If applicable, you can link to this but also important to explain the differences. Or we may need a whole tutorial about how to use that class, preferably as a notebook in the paleopandas repo. We can then fold it into our PyleoTutorials.

@CommonClimate CommonClimate merged commit 4b4fa90 into LinkedEarth:Development Feb 23, 2023
@kcpevey kcpevey deleted the doc_strings branch February 27, 2023 15:58
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.

2 participants