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

[ITensors] Index filtering docstring improvements #1171

Merged
merged 2 commits into from
Aug 4, 2023
Merged

Conversation

emstoudenmire
Copy link
Collaborator

Add list of most common keyword arguments to functions for filtering indices (commoninds, uniqueinds, etc.). Currently it's not so obvious to users what keyword arguments could be passed, as shown by a recent forum post.

@emstoudenmire
Copy link
Collaborator Author

@mtfishman please let me know if these docstrings look accurate to you

@mtfishman
Copy link
Member

mtfishman commented Aug 2, 2023

Looks good to me, thanks. One suggestion I would have is defining a function:

function index_filter_kwargs_docstring()
  return """
  Optional keyword arguments:
  * tags::String - a tag name or comma separated list of tag names that the returned indices must all have
  * plev::Int - common prime level that the returned indices must all have
  * inds - Index or collection of indices. Returned indices must come from this set of indices.
  """
end

and interpolate that into the other docstrings with $(index_filter_kwargs_docstring()) so if we want to change or update it we just need to update it in one place.

@emstoudenmire
Copy link
Collaborator Author

Great, yes I was thinking that very same thing but couldn't remember quite how to do it. I'll make that change.

@mtfishman mtfishman changed the title Index filtering docstring improvements [ITensors] Index filtering docstring improvements Aug 3, 2023
@emstoudenmire
Copy link
Collaborator Author

@mtfishman ok just made that change

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2023

Codecov Report

Merging #1171 (8ee3eba) into main (73bc07e) will decrease coverage by 5.14%.
Report is 13 commits behind head on main.
The diff coverage is 92.85%.

❗ Current head 8ee3eba differs from pull request most recent head a13b205. Consider uploading reports for the commit a13b205 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #1171      +/-   ##
==========================================
- Coverage   59.52%   54.39%   -5.14%     
==========================================
  Files          87       87              
  Lines        8361     8321      -40     
==========================================
- Hits         4977     4526     -451     
- Misses       3384     3795     +411     
Files Changed Coverage Δ
src/ITensors.jl 100.00% <ø> (ø)
src/mps/abstractprojmpo.jl 0.00% <ø> (-86.33%) ⬇️
src/physics/autompo/opsum_to_mpo_qn.jl 0.00% <0.00%> (-97.64%) ⬇️
src/itensor.jl 80.59% <100.00%> (+31.42%) ⬆️
src/oneitensor.jl 100.00% <100.00%> (ø)

... and 68 files with indirect coverage changes

@mtfishman mtfishman merged commit 32cc44f into main Aug 4, 2023
7 checks passed
@mtfishman mtfishman deleted the inds_docs branch August 4, 2023 12:40
@mtfishman
Copy link
Member

Thanks!

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