Skip to content

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Nov 13, 2023

Run a few sphinx doc tests:

  • html with -W make warnings tests error
  • linkcheck check if we are linking to any dead links
  • doctest run the snippets and check the output result
  • spelling check English and consistency spellings Missing a good spell-checker

Closes #359

@LecrisUT LecrisUT self-assigned this Nov 13, 2023
@LecrisUT LecrisUT added enhancement Significant ehancements documentation labels Nov 13, 2023
@LecrisUT LecrisUT force-pushed the ci/doc-test branch 2 times, most recently from 75eb49b to 36f8d18 Compare November 13, 2023 14:08
@LecrisUT LecrisUT mentioned this pull request Nov 13, 2023
@LecrisUT LecrisUT force-pushed the ci/doc-test branch 3 times, most recently from c76d0cd to c2391b0 Compare November 14, 2023 10:31
@LecrisUT LecrisUT requested review from atztogo and lan496 November 14, 2023 10:37
@LecrisUT LecrisUT marked this pull request as ready for review November 14, 2023 10:37
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Nov 14, 2023

@lan496 @atztogo I need your feedback about 00255f1. Do you prefer a different form for the prefix? Also all of the {cite} should be changed so that they redirect to this bib page.

Other than that I could use the help with the linkcheck since there are dead links

Oh, and also, we need review of the spellcheck

Copy link
Collaborator

@atztogo atztogo left a comment

Choose a reason for hiding this comment

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

I will follow @lan496 and @LecrisUT's choice.

@lan496
Copy link
Member

lan496 commented Nov 15, 2023

@LecrisUT

Do you prefer a different form for the prefix?

I have no preference. LGTM.

Also all of the {cite} should be changed so that they redirect to this bib page.

What should we do?

@lan496
Copy link
Member

lan496 commented Nov 15, 2023

As for linkcheck https://github.com/spglib/spglib/actions/runs/6862568901/job/18660561711?pr=361

@lan496
Copy link
Member

lan496 commented Nov 15, 2023

spelling looks too noisy. IMO, we do not have to introduce spelling in this PR.

@lan496
Copy link
Member

lan496 commented Nov 15, 2023

(Note for me)

@LecrisUT
Copy link
Collaborator Author

spelling looks too noisy. IMO, we do not have to introduce spelling in this PR.

I think we should address these issues here. Check e93df28 for how we can address them. There are a few issues present:

  • spelling errors: Fix them
  • unrecognized spellings, e.g. polarization: Just add them to spelling_wordlist.txt
  • mismatch between en-US and en-UK. We should choose one and stick with it
  • spelling in reference: Using {cite} that links to References.md will fix this (Releases.md and References.md are ignored)
  • convention names: cmake -> CMake. Need to be evaluated individually
  • maths and code names used in text. For the most case they should be converted to `` or $$. There are edge cases like gcc, and clang

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ba444f4) 83.86% compared to head (8d68b4d) 83.86%.
Report is 9 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #361   +/-   ##
========================================
  Coverage    83.86%   83.86%           
========================================
  Files           24       24           
  Lines         8180     8180           
========================================
  Hits          6860     6860           
  Misses        1320     1320           
Flag Coverage Δ
c_api 77.59% <ø> (ø)
fortran_api 56.15% <ø> (ø)
python_api 80.38% <ø> (ø)
unit_tests 1.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LecrisUT LecrisUT force-pushed the ci/doc-test branch 2 times, most recently from 0ce0edf to ce9c798 Compare November 15, 2023 13:18
@LecrisUT
Copy link
Collaborator Author

I have addressed all of the linkcheck and citation issues. The only remaining issue is the spelling, and I think we should discuss and fix them here. Most of them I have made exceptions, the remaining ones, please review and address, e.g. kpoints should be $k$-points, magmoms -> magnons, en-US or en-UK style, etc.?

@lan496
Copy link
Member

lan496 commented Nov 16, 2023

Thanks! The linkcheck looks nice.
But, I am still skeptical about spelling. The whitelist-based check is too strict. If there were a blacklist-based linter (like "centering" v.s. "centring"), I think it would be better.
How about making spelling not raise errors but just warnings (I know it goes agaist CI)?

Anyway, these words should be added to the wordlist.

spacegroup
schoenflies
kpoints
qpoints
monoclinic
translationengleiche
klessengleichesubgroup
magmom
magmoms
supergroup
(persons' names)

Let's use en-US style and use "centering" and "centered"

@LecrisUT
Copy link
Collaborator Author

The whitelist-based check is too strict. If there were a blacklist-based linter

I don't think a blacklist linter would make sense because there are infinite ways that words can be misspelled. On the other hand, the whilelist tells it that someone has reviewed and made a decision on what the style-guide is.

How about making spelling not raise errors but just warnings (I know it goes agaist CI)?

Indeed this one is a one-time difficulty, which we have to address. I don't like having warnings without addressing them, just like the -Wall right now.

About the other spellings I want to clarify a few things:

  • what are the definitions and sources for: magmom, translationengleiche, klessengleichesubgroup. Are these specific to one source or are they more generally used.
    My plan for these, since they are more niche than the other ones is to link these to an external page definition.
    • About magmom it feels too similar to magnon, if you can explain in more detail if there is any connection between these
  • kpoints and qpoints I prefer making them $k$-points and $Q$-points. Do you have a preference for non-latex types? Is $k$ and $Q$ ok like that, or should we use a different capitalization?
    • k-qpoints what is this one supposed to be?

@lan496
Copy link
Member

lan496 commented Nov 17, 2023

I am concerned that the whitelist for spelling contains only common vocabulary although spglib assumes some familiality with first principle calculations and crystallography, whose specific terminologies are not included in the list. If CI raised the barrier for writing documents, I would like to avoid that.

I don't like having warnings without addressing them, just like the -Wall right now.

Indeed. I totally agree that it is not a decisive solution.

Word definitions

  • magmom: abbreviation of magnetic moment, coming from VASP's keyword
  • translationengleiche and klessengleiche subgroup: terminology in crystallography https://www.cryst.ehu.es/html/cryst/help_pop-up/t_and_k_subgroups.html. These words are translated into t-subgroup and k-subgroup in English, but I think these are not usually used. Sorry, the latter should be klassengleiche.
  • kpoints, qpoints: That's a difficult question. I think these words are treated as variables in the document. So, kpoints and qpoints?
  • k-qpoints: Maybe it represents kpoints and qpoints. @atztogo Is it correct?

@LecrisUT
Copy link
Collaborator Author

If CI raised the barrier for writing documents, I would like to avoid that.

Indeed, that is also why it is only on the GitHub CI and not on pre-commit. It should not be considered as a hard barrier for the writer to check, but I think it is useful to have it for the reviewer. E.g. in this case there are some words that are niche even to the first-principles community, in which case I think it is helpful to link the reader to the external definition.

  • kpoints, qpoints: That's a difficult question. I think these words are treated as variables in the document. So, kpoints and qpoints?

Oh, good call, I will look more carefully in the context of these.

Great, I can handle these with that information. I'll get back to this on the weekend.

@atztogo
Copy link
Collaborator

atztogo commented Nov 17, 2023

  • k-qpoints: Maybe it represents kpoints and qpoints. @atztogo Is it correct?

Thanks for pointing out this, @LecrisUT and @lan496. This is a typo. This will be changed to k-points. All q-point(s) should be also replaced by k-points(s).

The purpose to develop Kpoints related functions was to use them for my phonon calculation. In the old time, I never though that spglib would become so popular, so I was not careful about it, and so I could write such the confusing documentation. Now I want to prepare another library specific for reciprocal space mesh, and made those functions in spglib deprecated at future version, because the grid generated by current mesh setting can break crystallographic point group. I wrote those theoretical detail in the paper, https://iopscience.iop.org/article/10.1088/1361-648X/acd831.

@lan496
Copy link
Member

lan496 commented Nov 17, 2023

It should not be considered as a hard barrier for the writer to check, but I think it is useful to have it for the reviewer.

I see. I am frighted the default spelling warns every person's name. Is there any better method?

For terminology in crystallography, it will be nice to add all words in IUCr's dictionary. I'll prepare it.

@LecrisUT
Copy link
Collaborator Author

It should not be considered as a hard barrier for the writer to check, but I think it is useful to have it for the reviewer.

I see. I am frighted the default spelling warns every person's name. Is there any better method?

For terminology in crystallography, it will be nice to add all words in IUCr's dictionary. I'll prepare it.

I would say, we don't need to proactively add all dictionary words, it is trivial to add them as they become relevant.

The names is indeed unfortunate, and I don't know of good ways to add them. Contributor names should automatically be handled at least (I think taken from git? Not sure how that works). A small silver-lining to this, at least we would not be misspelling people's names :D

@lan496
Copy link
Member

lan496 commented Nov 17, 2023

Looks like taken from git history, https://sphinxcontrib-spelling.readthedocs.io/en/latest/customize.html#word-filters.

Oh, I just collect words from the page (never mind it's an easy scraping job).

word list
Abelian group
absolute structure
absorption coefficient
absorption edge
absorption threshold
acceptance domain
affine isomorphism
affine mapping
allotwin
anomalous absorption
anomalous dispersion
anomalous scattering
anomalous transmission
aperiodic crystal
aristotype
arithmetic crystal class
asymmetric unit
atomic modulation function
atomic scattering factor
atomic surface
automorphism
Bieberbach group
Bijvoet pair
binary operation
Borrmann effect
Bragg angle
Bragg R factor
Bragg's law
Bravais arithmetic class
Bravais class
Bravais group
Bravais lattice
Bravais law
Bravais-Miller indices
Brightness
Brilliance
Brillouin zone
Cartesian product
cell-twinning
center
centralizer
centred lattice
charge flipping
chirality
chiral space group
CIF
co-crystal
complex
composition plane
conjugacy class
constrained refinement
conventional cell
corresponding twins
coset
Cromer–Mann coefficients
cross-section
crystal
crystal class
crystal family
crystal pattern
crystal structure
crystal system
crystallographic basis
Crystallographic Information File
Crystallographic Information Framework
crystallographic orbit
crystallographic symmetry
Curie laws
cylindrical system
D centred cell
DDL
derivative structure
detective quantum efficiency (DQE)
diadochy
dictionary definition language
difference Patterson map
diffraction symbol
direct lattice
direct methods
direct product
direct space
direction indices
displacive modulation
domain of influence
double coset
dual basis
dual-space phase retrieval algorithms
dynamic range
dynamical diffraction
dynamical theory
eigensymmetry
electrocaloric effect
electron density map
epitaxy
Euclidean mapping
euhedral
Ewald sphere
extended X-ray absorption fine structure (EXAFS)
extinction symbol
extinctions
F(000)
factor group
family structure
Fermi energy
Fermi level
ferroelastics
ferroelectrics
ferroics
ferromagnets
fixed-point-free space group
Flack parameter
flux
flux density
form
free R factor
Friedel's law
Friedel pair
frieze group
geometric crystal class
geometric element
gnomonic projection
Goldschmidt tolerance factor
group
group homomorphism
group isomorphism
groupoid
H centred cell
habit
Harker section
heavy-atom method
hemihedry
hemitropy
Hermann-Mauguin symbols
high energy resolution fluorescence detection (HERFD)
holohedry
hybrid input-output algorithm
hybrid twin
image
incommensurate composite crystal
incommensurate magnetic structure
incommensurate modulated structure
inelastic X-ray scattering (IXS)
integral reflection conditions
International Union of Crystallography
interplanar spacing
inversion twin
involution
isogonal
isometry
isomorphous crystals
isomorphous replacement
isostructural crystals
isotypic crystals
kinematical theory
lattice
lattice complex
lattice system
Laue class
Laue equations
Laue indices
law of rational indices
law of the constancy of interfacial angles
layer group
lepton
limiting complex
linear attenuation coefficient
local symmetry
Lorentz–polarization correction
Mallard's law
mapping
mass attenuation coefficient
maximum likelihood
merohedral
merohedry
mesh
mesocrystal
metric specialization
metric tensor
Miller indices
modular crystal structure
modulated crystal structure
molecular replacement
morphotropism
mosaic crystal
multi-electron excitation
multiwavelength anomalous diffraction (MAD)
Neumann's principle
noncrystallographic symmetry
normalizer
normal subgroup
OD structure
ogdohedry
optical resolution
order (group theory)
orientation matrix
orthohexagonal
parent clamping approximation
partial structure
partial symmetry
Patterson methods
Patterson vector
Pearson symbol
phase of a modulation
phase problem
phase transition
piezoelectricity
pleochroism
point configuration
point group
point space
point symmetry
polar lattice
polyamorphism
polymorphism
polytypism
powder
preferred orientation
primary extinction
primitive basis
primitive cell
priority rule
pseudo symmetry
pyroelectricity
Quasicrystal
Quasiperiodicity
R factor
real-space correlation coefficient
real-space residual
reciprocal lattice
reciprocal space
reduced cell
refinement
reflection conditions
reflection twin
Renninger effect
resolution
resonant scattering
restrained refinement
Rietveld method
right-handed quartz
rod group
rotation twin
Sayre equation
secondary extinction
secondary process detection
selection rules
selective merohedry
semidirect product
serial reflection conditions
sharpened Patterson function
site symmetry
Sohncke groups
space group
spherical system
stabilizer
stacking fault
statistical descriptors
stereographic projection
structure amplitude
structure determination
structure factor
structure-factor coefficient
subcell
subgroup
sublattice
subperiodic crystal
subperiodic group
substructure
supercell
supergroup
superlattice
superposition method
superspace
superspace group
superspace point group
superstructure
symmetry element
symmetry operation
symmorphic space groups
synneusis
systematic absences
tetartohedry
thermal expansion
TLQS twinning
TLS twinning
torsion-free space groups
twin
twin (diffraction pattern of)
twin element
twin index
twin lattice
twin law
twin obliquity
twin operation
twinning
twinning (effects of)
twinning (endemic conditions of)
twinning by inversion
twinning by merohedry
twinning by metric merohedry
twinning by pseudomerohedry
twinning by reticular merohedry
twinning by reticular polyholohedry
twinning by reticular pseudomerohedry
unit cell
vector module
vector space
VLD
Voronoi domain
Weber indices
Weiss parameters
Weiss zone law
Weissenberg complex
Wigner-Seitz cell
Wyckoff position
Wyckoff set
X-ray absorption fine structure (XAFS)
X-ray absorption spectroscopy (XAS)
X-ray emission spectroscopy (XES)
X-ray excited optical luminescence (XEOL)
zonal reflection conditions
zone
zone axis

@lan496
Copy link
Member

lan496 commented Nov 17, 2023

Well, as you already have worked on this convincing PR, I am OK to proceed in this direction. Let me think again if the new linters cause confusion on possible contributors.

@LecrisUT
Copy link
Collaborator Author

Looks like taken from git history, https://sphinxcontrib-spelling.readthedocs.io/en/latest/customize.html#word-filters.

Cool, thanks for checking. Make sure you don't have a typo in your git author :D

Oh, I just collect words from the page (never mind it's an easy scraping job).

It's a bit nuanced because it doesn't seem to pick up on multiple words. I have tracked down that the spelling builder is using PyEnchant as the backend to parse these files. I will look into it if there is some special syntax that is needed

@LecrisUT
Copy link
Collaborator Author

For now I have dropped sphinx-spelling. I have added codespell, which works ok as a white-list spell-checker, but it misses a lot of them. PyEnchant would be nice to add on top of it, but its interface is not good enough yet.

@lan496
Copy link
Member

lan496 commented Jan 20, 2024

@LecrisUT Can you rebase develop branch? Recently, I've changed python API docs a lot.

@LecrisUT
Copy link
Collaborator Author

@LecrisUT Can you rebase develop branch? Recently, I've changed python API docs a lot.

This one has already been rebased to include them, I'll do it again after the F38 PR

@lan496
Copy link
Member

lan496 commented Jan 20, 2024

I guess #393 is still not reflected.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

I guess #393 is still not reflected.

Still haven't figured how to convince github that the commits are already present, maybe with merged commits 🤷

Copy link
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

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

LGTM!

@lan496 lan496 merged commit c168e89 into spglib:develop Jan 20, 2024
@LecrisUT LecrisUT deleted the ci/doc-test branch January 22, 2024 09:37
@LecrisUT LecrisUT added this to the 2.3 milestone Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation enhancement Significant ehancements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Documentation: wrap code snippets with doctest
4 participants