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 documentation for dynamical.ops and dynamical.handlers #439

Merged
merged 8 commits into from
Dec 8, 2023

Conversation

azane
Copy link
Contributor

@azane azane commented Dec 7, 2023

Partly addresses #337 by adding documentation for simulate, LogTrajectory StaticBatchObservation and all user-facing interruptions.

Copy link
Contributor Author

@azane azane Dec 7, 2023

Choose a reason for hiding this comment

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

Not sure if we actually want this, but this was the cleanest way I could find (after looking only a little bit) to document these type aliases. Happy to remove if not desired. Am currently linking to these from the simulate docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I build the docs I get this warning.

checking consistency... /Users/sam-basis/Desktop/Research/chirho/docs/source/type_aliases.rst: WARNING: document isn't included in any toctree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I was thinking that we don't want it in the doctree, and just to be linked? Not sure if there's a way to supress the warning...or if we should just include it in the doctree.

@azane azane requested review from SamWitty and eb8680 December 7, 2023 23:44
@SamWitty
Copy link
Collaborator

SamWitty commented Dec 8, 2023

@azane , could you address the lint failure? Thanks!

@SamWitty SamWitty added documentation Improvements or additions to documentation module:dynamical status:awaiting response Awaiting response from creator labels Dec 8, 2023
@azane azane added status:awaiting review Awaiting response from reviewer and removed status:awaiting response Awaiting response from creator labels Dec 8, 2023
@azane
Copy link
Contributor Author

azane commented Dec 8, 2023

@SamWitty done

@eb8680
Copy link
Contributor

eb8680 commented Dec 8, 2023

@azane have you built docs locally and confirmed that everything renders correctly?

@SamWitty
Copy link
Collaborator

SamWitty commented Dec 8, 2023

Confirming that operations and handlers render correctly when I build locally:

Screenshot 2023-12-08 at 10 57 11 AM

However, as my comment above about the type aliases warning implies, I don't think the type aliases are currently included in the docs build.

@SamWitty
Copy link
Collaborator

SamWitty commented Dec 8, 2023

One thing I notice in the above rendering is that the State type alias has been evaluated away. E.g. rather than State[T] it shows up as Mapping[str, T]. This is maybe fine, but it will make things confusing if we use the type alias name directly in the documentation itself. The example I showed above has State[T] listed as the return type.

@SamWitty
Copy link
Collaborator

SamWitty commented Dec 8, 2023

@azane , I'll look into a fix for this. If you can come up with one as well feel free.

@azane
Copy link
Contributor Author

azane commented Dec 8, 2023

@azane have you built docs locally and confirmed that everything renders correctly?

Yes — sorry should have mentioned that!

@azane
Copy link
Contributor Author

azane commented Dec 8, 2023

One thing I notice in the above rendering is that the State type alias has been evaluated away. E.g. rather than State[T] it shows up as Mapping[str, T]. This is maybe fine, but it will make things confusing if we use the type alias name directly in the documentation itself. The example I showed above has State[T] listed as the return type.

@SamWitty ya, so this happens in the actual signature but I haven't been able to figure out how to "unpack" the alias in the docstring itself. If you click the hyperlinks there it takes you to the type alias page that shows the underlying type.

My primary question here is whether we just want to take those non-unpacked type aliases out of the docstring. I think I am leaning toward that at the moment...

simulate btw is the only docstring I've done this in, btw.

@azane
Copy link
Contributor Author

azane commented Dec 8, 2023

Also — forgot to go through and run all the code blocks to make sure they work. Doing that now...

done

@eb8680 eb8680 changed the title Add Documentation Add documentation for dynamical.ops and dynamical.handlers Dec 8, 2023
@SamWitty
Copy link
Collaborator

SamWitty commented Dec 8, 2023

One thing I notice in the above rendering is that the State type alias has been evaluated away. E.g. rather than State[T] it shows up as Mapping[str, T]. This is maybe fine, but it will make things confusing if we use the type alias name directly in the documentation itself. The example I showed above has State[T] listed as the return type.

@azane and I figured out a workaround, but will plan on submitting a follow-up
PR as this issue is pervasive throughout all of the documentation, not just dynamical systems.

Copy link
Collaborator

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

Looks great! We can nitpick about the verbiage in future PRs. Also, we'll make a global change to how type aliases are used in docs for all modules as a separate PR, as that applied beyond just this block of documentation.

@SamWitty SamWitty merged commit 6f7eb0a into master Dec 8, 2023
7 checks passed
@SamWitty SamWitty deleted the az-dynsys-docs branch December 8, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation module:dynamical status:awaiting review Awaiting response from reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants