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

docs: reorganize function reference #662

Merged
merged 58 commits into from
Mar 13, 2023
Merged

Conversation

maelle
Copy link
Contributor

@maelle maelle commented Feb 6, 2023

Fix #659
Based on #660

src/Makevars Outdated Show resolved Hide resolved
man/roxygen/meta.R Outdated Show resolved Hide resolved
man/roxygen/meta.R Outdated Show resolved Hide resolved
@maelle

This comment was marked as resolved.

@maelle

This comment was marked as resolved.

@maelle
Copy link
Contributor Author

maelle commented Feb 6, 2023

I'm constantly writing family as "famility" now 😹

@szhorvat
Copy link
Member

szhorvat commented Feb 6, 2023

is assortativity a structural property?

Yes, we can call it that, and I do suggest placing it into that section.

Frankly, all properties are "structural properties" and that section should eventually be broken up, once we get there. Unfortunately, at the moment that section is used as a bit of a dumping ground.

@szhorvat
Copy link
Member

szhorvat commented Feb 6, 2023

If you have any questions about the various concepts, feel free to ask.

@szhorvat
Copy link
Member

szhorvat commented Feb 6, 2023

I think that the documentation of IGraph/M, the Mathematica interface of igraph, is structured a bit better (and a bit more granularly than the C library's docs). You might find it helpful as a guide. http://szhorvat.net/mathematica/IGDocumentation/ Apologies about the giant doc page ... I just don't have the time, and Mathematica's doc tools are not there yet, so I needed to do a lot from scratch.

@ntamas
Copy link
Member

ntamas commented Feb 16, 2023

There are some deprecated layout functions in the "Versions" section. These don't belong there.

I don't see them either; I guess this point is now obsolete.

I think consolidating the lifecycle workflow (using the lifecycle package?) would make sense.

Totally agree, but I think that is not in the scope of this PR; we could do it separately in another PR. If you think that it's easier to make the deprecated functions disappear using the lifecycle package, just leave them as is for now and we'll update the docs when we adopt the usage of the lifecycle package.

Does layout.grid.3d count as deprecated or not?

Yes, it's deprecated.

Should "Scan statistics" be a subsection of "Structural properties"? Thoughts?

Let's make it a subsection.

@szhorvat
Copy link
Member

console() can go under "Interactive", it's fixed now.

  • There are some deprecated layout functions in the "Versions" section. These don't belong there. I would actually suggest removing these completely from the docs, as people shouldn't be using them (@ntamas, are you okay with that?)

Which ones?

I guess these are not really under "Versions", they just come at the end and appear to be under "Versions"? See here:

https://maelle.github.io/rigraph/reference/index.html#versions

I think everything should be excluded, except:

  • graph_version()
  • upgrade_graph()
  • igraph_version()
  • igraph_test()
  • console() (move to "Inrteractive")
  • %>% (but do we need this in the igraph docs?)

If this section is renamed to something else, e.g. "System", then igraph_test() will fit here just fine. Otherwise it needs a new home.

@maelle
Copy link
Contributor Author

maelle commented Feb 20, 2023

I had make a mistake in the pkgdown config syntax so all functions I had listed under "Internal" still showed up, sorry about that.

Are there further tweaks to make?

@maelle
Copy link
Contributor Author

maelle commented Mar 6, 2023

@krlmlr apart from the conflicts, ok to merge this?

@szhorvat
Copy link
Member

szhorvat commented Mar 6, 2023

Can we rename "Stochastic constructors – Random graph models (games)" to simply "Stochastic constructors (random graph models)"? I know I suggested the current title, but I just realize that the new dot-free names no longer use the "game"-terminology. So there's no reason to put that into the title.

Looking forward to this getting merged.

@ntamas
Copy link
Member

ntamas commented Mar 6, 2023

@maelle This conflicts with the base branch - is there an easy way to resolve the conflicts so this can be merged?

@szhorvat
Copy link
Member

szhorvat commented Mar 6, 2023

I'm looking through this once more, and here are a few more comments. @maelle I could make some of these changes myself, but I didn't want to interfere with your PR. Let me know if you prefer that I commit to this PR directly in the future.

  • "Spectral Coarse Graining" heading -> "Spectral coarse graining" (consistent case)
  • arpack() and arpack_defaults should not be in "Centrality measures". These can have their own top-level section.
  • spectrum() also doesn't belong to "Centralirty measures". It's an odd one out, but it can go in top-level "Structural properties"
  • "Hierarchical random graph functions" -> "Hierarchical random graphs" (it's nice if most headings fit on a single line without wrapping). The original uses the same title too.
  • "Emebdding" -> "Spectral embedding" (useful to be specific here)
  • diameter() goes under "Paths"
  • is_chordal() and max_cardinality_search() are both in the wrong place. They could go in top-level structural properties, or their own subsection on "Chordal graphs"
  • is_dag() shouls not be under "Paths". It can be included both in "Structural properties" and "Graph cycles".
  • feedback_arc_set() and girth() should appear in "Graph cycles" as well, in addition to their current location.
  • I'm not sure what printr is. Is it supposed to be in the docs @ntamas ?

This is probably not a subject of this PR, but hub_score() and authority_score() should share a single documentation page. Future versions will have a single function to compute both of these at the same time, in a way that the result match.

This reorganization is of course a never-ending task ... But this new doc overview page makes it much easier to find and correct issues.

@szhorvat szhorvat removed their request for review March 6, 2023 20:01
@maelle
Copy link
Contributor Author

maelle commented Mar 7, 2023

@maelle This conflicts with the base branch - is there an easy way to resolve the conflicts so this can be merged?

@ntamas I don't think they are huge conflicts but the interface tells me that only those with write access can merge the PR?

@krlmlr
Copy link
Contributor

krlmlr commented Mar 7, 2023

I resolved the conflicts by merging main into this branch and rerunning devtools::document() .

@maelle: Would you like to do one more iteration here?

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #662 (9942f1e) into main (8f8e65a) will decrease coverage by 0.17%.
The diff coverage is n/a.

❗ Current head 9942f1e differs from pull request most recent head 2d25e9f. Consider uploading reports for the commit 2d25e9f to get more accurate results

@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
- Coverage   53.81%   53.65%   -0.17%     
==========================================
  Files         355      356       +1     
  Lines       73313    73544     +231     
==========================================
+ Hits        39457    39458       +1     
- Misses      33856    34086     +230     
Impacted Files Coverage Δ
R/aaa-auto.R 78.81% <ø> (ø)
R/attributes.R 78.00% <ø> (ø)
R/centrality.R 74.75% <ø> (ø)
R/community.R 74.63% <ø> (ø)
R/decomposition.R 80.00% <ø> (ø)
R/embedding.R 100.00% <ø> (ø)
R/epi.R 91.13% <ø> (ø)
R/eulerian.R 100.00% <ø> (ø)
R/flow.R 79.13% <ø> (ø)
R/games.R 76.37% <ø> (ø)
... and 11 more

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@maelle
Copy link
Contributor Author

maelle commented Mar 13, 2023

I changed the default branch which was causing quite a few problems when trying to fix conflicts.

Rebasing didn't work as I expected (I'm stuck a conflict in cigraph/), so I'll try merging as indicated above.

Merge branch 'main' into reference-reorg

# Conflicts:
#	R/centrality.R
#	man/authority_score.Rd
@maelle
Copy link
Contributor Author

maelle commented Mar 13, 2023

@ntamas @szhorvat time to merge or time for a few last tweaks? 😸

Merge branch 'main' into reference-reorg

# Conflicts:
#	_pkgdown.yml
#	man/dot-apply_modifiers.Rd
#	man/dot-extract_constructor_and_modifiers.Rd
@maelle maelle requested review from szhorvat and ntamas March 13, 2023 14:30
@ntamas ntamas merged commit 00705ce into igraph:main Mar 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve pkgdown configuration for reference index
4 participants