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 example and improve stream detrend docs #2237

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@d-chambers
Member

d-chambers commented Oct 9, 2018

What does this PR do?

Tries to improve the docstring for obspy.core.stream.Stream.detrend method by rewording slightly and adding an example.

Why was it initiated? Any relevant Issues?

see #2055

+DOCS

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 10, 2018

Member

Thanks! Would you mind adding the :param options: as proposed by OP in #2055, I think he has a point there.

Member

megies commented Oct 10, 2018

Thanks! Would you mind adding the :param options: as proposed by OP in #2055, I think he has a point there.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 11, 2018

Member

Ok, sure. I personally don't think we should have to explain the ** mechanics like this in every function/method that uses it though. It would become very tedious to read and write very quickly.

Member

d-chambers commented Oct 11, 2018

Ok, sure. I personally don't think we should have to explain the ** mechanics like this in every function/method that uses it though. It would become very tedious to read and write very quickly.

@trichter

This comment has been minimized.

Show comment
Hide comment
@trichter

trichter Oct 11, 2018

Member

The code block does not render correctly.
Here is a link to the relevant doc.

Member

trichter commented Oct 11, 2018

The code block does not render correctly.
Here is a link to the relevant doc.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 13, 2018

Member

I cherry-picked the relevant commits onto master to get rid of the merge commit. When docs are done building this should be ready to review/merge if the example renders correctly.

Member

d-chambers commented Oct 13, 2018

I cherry-picked the relevant commits onto master to get rid of the merge commit. When docs are done building this should be ready to review/merge if the example renders correctly.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 15, 2018

Member

Actually, on a second look, the detail descriptions and example should probably go to the API docs of Trace.detrend(), I think. For all methods on Stream that are basically just doing a for tr in self.traces: ..., we always have the Stream docs pretty much empty, pointing to the corresponding Trace method, mainly to avoid duplicated maintenance efforts (which are also prone to get broken on changes).

Member

megies commented Oct 15, 2018

Actually, on a second look, the detail descriptions and example should probably go to the API docs of Trace.detrend(), I think. For all methods on Stream that are basically just doing a for tr in self.traces: ..., we always have the Stream docs pretty much empty, pointing to the corresponding Trace method, mainly to avoid duplicated maintenance efforts (which are also prone to get broken on changes).

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 15, 2018

Member

@megies, sure, I don't mind moving the changes to trace.detrend. I think we should keep the in-place warning on Stream though. Also, there are other stream methods with fairly extensive docs/examples such as attach_response, normalize, std, interpolate, decimate, etc. Should we look and pushing most of the detail in the docs for those methods to their corresponding trace methods?

Member

d-chambers commented Oct 15, 2018

@megies, sure, I don't mind moving the changes to trace.detrend. I think we should keep the in-place warning on Stream though. Also, there are other stream methods with fairly extensive docs/examples such as attach_response, normalize, std, interpolate, decimate, etc. Should we look and pushing most of the detail in the docs for those methods to their corresponding trace methods?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 15, 2018

Member

@megies, sure, I don't mind moving the changes to trace.detrend. I think we should keep the in-place warning on Stream though. Also, there are other stream methods with fairly extensive docs/examples such as attach_response, normalize, std, interpolate, decimate, etc. Should we look and pushing most of the detail in the docs for those methods to their corresponding trace methods?

I wanna say yes. I think for a proper solution we should try to find some automated way to deal with this. We could use decorators to mark functions as being in-place and only document things at the Trace level and have some sphinx trickery to also render it with the equivalent Stream functions.

Member

krischer commented Oct 15, 2018

@megies, sure, I don't mind moving the changes to trace.detrend. I think we should keep the in-place warning on Stream though. Also, there are other stream methods with fairly extensive docs/examples such as attach_response, normalize, std, interpolate, decimate, etc. Should we look and pushing most of the detail in the docs for those methods to their corresponding trace methods?

I wanna say yes. I think for a proper solution we should try to find some automated way to deal with this. We could use decorators to mark functions as being in-place and only document things at the Trace level and have some sphinx trickery to also render it with the equivalent Stream functions.

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Oct 15, 2018

Contributor

docrep might be useful here - it would be nice to not have to jump from a Stream method to a Trace method. I haven't tried docrep, so I don't know if it will work/be useful, and it would add a dependency.

Contributor

calum-chamberlain commented Oct 15, 2018

docrep might be useful here - it would be nice to not have to jump from a Stream method to a Trace method. I haven't tried docrep, so I don't know if it will work/be useful, and it would add a dependency.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 15, 2018

Member

docrep might be useful here - it would be nice to not have to jump from a Stream method to a Trace method. I haven't tried docrep, so I don't know if it will work/be useful, and it would add a dependency.

That's fine - I think we can make this a doc building only dependency. We'd just try to import it and if its not available we provide dummy decorators.

This look really nice - I think I'll use this for a bunch of my things. Thanks for making me aware of it!

Member

krischer commented Oct 15, 2018

docrep might be useful here - it would be nice to not have to jump from a Stream method to a Trace method. I haven't tried docrep, so I don't know if it will work/be useful, and it would add a dependency.

That's fine - I think we can make this a doc building only dependency. We'd just try to import it and if its not available we provide dummy decorators.

This look really nice - I think I'll use this for a bunch of my things. Thanks for making me aware of it!

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Oct 15, 2018

Member

The only subtlety here is that we will want to be careful not to ruin the usefulness of docstrings in the REPL. If we don't make docrep a hard dependency this would be easy to do.

Member

d-chambers commented Oct 15, 2018

The only subtlety here is that we will want to be careful not to ruin the usefulness of docstrings in the REPL. If we don't make docrep a hard dependency this would be easy to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment