Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix #25032 by reading gallery ordering from a txt file in galleries folder#27991
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
doc/sphinxext/gallery_order.py Outdated
| #plot_types_order = [ | ||
| # '../galleries/plot_types/basic', | ||
| # '../galleries/plot_types/stats', | ||
| # '../galleries/plot_types/arrays', | ||
| # '../galleries/plot_types/unstructured', | ||
| # '../galleries/plot_types/3D', | ||
| # UNSORTED | ||
| #] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if this is no longer needed, it can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed them in latest commit
| plot_types_directory = "../galleries/plot_types/" | ||
| plot_types_order = get_ordering(plot_types_directory, include_directory_path=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does this solution generalize? (to the other folders?) I like that it looks relatively lightweight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes it does, because other folders are also structured in the same way asplot_types directory. Except for theusers_explain directory, because its not included inexplicit_order_folders but some of it's files are inlist_all. I'm not sure whyusers_explain directory is not included inexplicit_order_folders but because of this, i think its currently being sorted according to default values but that doesn't seem to totally line up with my understanding. because the sort keys returned byMplExplicitOrder will be UNSORTED thus defaulting to alphabetic ordering or LOC based ordering. But that doesn't seem to be the case when i'm going through theuser guide section in doc website. Any clarification on how user_explain is sorted will help me greatly.
Other than that, rest of the folders will work fine with my current implementation as far as i understand. I'll push an update soon with the modifications for other other folders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for not explaining this earlier, user_explain doesn't have a gallery landing page, instead it useshttps://github.com/matplotlib/matplotlib/blob/main/doc/users/index.rst
NirobNabil commentedMar 29, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Completely excludeduser guide andtutorials page from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
looking great, sorry for dropping the ball on this. for the flake errors, have you installed the pre-commit hooks?https://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks
They should autofix that error.
| file_path = os.path.join(dir, 'gallery_order.txt') | ||
| f = open(file_path, "r") | ||
| lines = [line.replace('\n', '') for line in f.readlines()] | ||
| ordered_list = [] | ||
| for line in lines: | ||
| if line == "unsorted": | ||
| ordered_list.append(UNSORTED) | ||
| else: | ||
| if include_directory_path: | ||
| ordered_list.append(os.path.join(dir, line)) | ||
| else: | ||
| ordered_list.append(line) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| file_path=os.path.join(dir,'gallery_order.txt') | |
| f=open(file_path,"r") | |
| lines= [line.replace('\n','')forlineinf.readlines()] | |
| ordered_list= [] | |
| forlineinlines: | |
| ifline=="unsorted": | |
| ordered_list.append(UNSORTED) | |
| else: | |
| ifinclude_directory_path: | |
| ordered_list.append(os.path.join(dir,line)) | |
| else: | |
| ordered_list.append(line) | |
| ordered_list= [] | |
| withopen(os.path.join(dir,'gallery_order.txt'),'r')asf: | |
| forlineinf.readlines(): | |
| line=line.rstrip("\n") | |
| ifline=="unsorted": | |
| ordered_list.append(UNSORTED) | |
| elifinclude_directory_path: | |
| ordered_list.append(os.path.join(dir,line)) | |
| else: | |
| ordered_list.append(line) |
um this is mostly stylistic, but it does remove one loop and I think you should be able to use context managers here
| def list_directory(parent_dir): | ||
| """Return list of sub directories at a directory""" | ||
| root, dirs, files = next(os.walk(parent_dir)) | ||
| return [os.path.join(root, dir) for dir in dirs] | ||
| # Gallery sections shall be displayed in the following order. | ||
| # Non-matching sections are inserted at the unsorted position | ||
| UNSORTED = "unsorted" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should maybe be moved to top of file if it's being used in the listing function above (or passed in as a variable)
| plot_types_directory = "../galleries/plot_types/" | ||
| plot_types_order = get_ordering(plot_types_directory, include_directory_path=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry for not explaining this earlier, user_explain doesn't have a gallery landing page, instead it useshttps://github.com/matplotlib/matplotlib/blob/main/doc/users/index.rst
| except FileNotFoundError: | ||
| # Fallback to ordering already defined in list_all | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can this be logged to sphinx just in case this ignores something that shouldn't be ignored?
@NirobNabil any bandwidth for getting back to this or can we label this as orphaned? |
timhoffm commentedSep 26, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Was there a design discussion on this? Note that since sphinx-gallery 0.16.0, subsection order can be a simple list with optionally one wildcard '*' (https://sphinx-gallery.github.io/stable/configuration.html#sorting-gallery-subsections). I believe the current ordering can be achieved without any code by:
Example: (click to expand)This simplifies the structure as it's purely declarative. We could even rid of our This is simple and may be good enough. The only downside is that sphinx-gallery does not have per-gallery order. All order information must be placed into the two lists If we really want individual config files (and are willing to write and maintain the code logic for that). I suggest to go with one structured file per gallery (e.g. YAML) so that a Footnotes |
@story645 I'm getting back to this tonight. sorry totally forgot about it in the inactivity. |
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Addresses#25032. Requires review for closing#25032
Modified
gallery_order.pyto read ordering of examples fromgalleries/plot_typesand its subdirectories. It adresses the issue described in#25032 but instead of reading the ordering from readme it's currently fetching ordering fromgallery_order.txtas described in the comments by @timhoffm and@story645.I only added the functionality for
galleries/plot_typesas a draft to gather feedback on the approach. After iterations if the implementation approach is finalized I can work on integrating with other galleries and also add the devel documentation as asked in#25032PR checklist