Raise TypeError for invalid cell

[lan496]

Closes #412 and #411

• Add naive type annotation for cell
• raise TypeError when cell does not have appropriate shapes

[lan496]

The current CI does not check on ubuntu-latest. That should be fixed.

[lan496]

@LecrisUT run-cmake on CI starts to fail. Can you take a look?
https://github.com/spglib/spglib/actions/runs/7763425441/job/21175525252?pr=420

[LecrisUT]

The current CI does not check on ubuntu-latest. That should be fixed.

Would need to use something like https://github.com/rscohn2/setup-oneapi. Not sure how well that action works.

[lan496]

I realize the annotation is not useful in keeping the cell's type as a naive tuple. I omit the type annotation in this PR and just add to emit exception for invalid cells.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a14e53e) 77.75% compared to head (7603cda) 83.59%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #420      +/-   ##
===========================================
+ Coverage    77.75%   83.59%   +5.84%     
===========================================
  Files           23       24       +1     
  Lines         7632     8119     +487     
  Branches      1605     1687      +82     
===========================================
+ Hits          5934     6787     +853     
+ Misses        1698     1332     -366     

Flag	Coverage Δ
c_api	74.44% <ø> (ø)
fortran_api	56.18% <ø> (ø)
python_api	79.74% <ø> (?)
unit_tests	13.52% <ø> (ø)

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.

[atztogo]

Thanks @lan496. LGTM.

[LecrisUT]

We should still add the cell type-hinting because from the typeerror messages it is unclear which position is which that raised the error

[lan496]

Yes, I agree that the current cell tuple is unclear in which elements represent what. I do not come up with a way to solve it with keep backward compatibility.

[LecrisUT]

I do not come up with a way to solve it with keep backward compatibility.

What you had before was a good backwards compatible approach, you just needed to add those into if TYPE_CHECKING as well

[lan496]

What you had before was a good backwards compatible approach, you just needed to add those into if TYPE_CHECKING as well

In this case, autodoc2 just shows spglib.Cell in document. If even we replace it, annotations like tuple[ArrayLike, ArrayLike, ArrayLike] convey almost no information.

[LecrisUT]

In this case, autodoc2 just shows spglib.Cell in document.

Yes, that is the intended output. What we should check is how to add those aliases to the documentation, either via autodoc2 or manually. I remember sphinx allows documenting global variables, maybe it uses a similar logic for that

[lan496]

autodoc2_replace_annotations will be the way for it. But I still do not expect it helps users.

[LecrisUT]

autodoc2_replace_annotations will be the way for it. But I still do not expect it helps users.

Can you remove the undoc so we can check everything that it tries to document?

[lan496]

After removing undoc, no link is still shown for spglib.Cell.
https://github.com/lan496/spglib/tree/cell-annotation

[lan496]

@LecrisUT
If we introduce the type aliases like

if TYPE_CHECKING:
    from collections.abc import Sequence

    from typing_extensions import TypeAlias

    Lattice: TypeAlias = Sequence[Sequence[float]]
    Positions: TypeAlias = Sequence[Sequence[float]]
    Numbers: TypeAlias = Sequence[int]
    Magmoms: TypeAlias = Sequence[float] | Sequence[Sequence[float]]
    Cell: TypeAlias = (
        tuple[Lattice, Positions, Numbers] | tuple[Lattice, Positions, Numbers, Magmoms]
    )

, I think the remained problem is in autodoc2's document.
Because autodoc2 does not seem to resolve the type aliases automatically, there will be two ways:

1. use autodoc2_replace_annotations
2. set autodoc2_annotations = False not to mislead users as if there exists a new type called spglib.Cell.

Now I think the option 2 is tolerable. What do you think?

[LecrisUT]

@lan496, Sorry for getting back at you late, I had to try out some stuff on my end. What I've found is that autodoc2 does document spglib.Cell if we move it outside of if TYPE_CHECKING, but things are a bit more complicated. We should research all autodocs more carefully:

• autodoc2:
  • It is mainly a glue between myst documents and sphinx autodoc
  • The main author has a lot of projects to attend to so development is slow atm
  • There are efforts to add the myst integration and other functionalities into sphinx-autoapi ¹
• sphinx-autoapi:
  • Closest compatibility with sphinx.ext.autodoc
  • It mainly generates documentation outside without having to import. But we are always installing spglib as part of pip install .[docs] so it makes little difference there
• sphinx.ext.autodoc (+ sphinx_autodoc_typehints or sphinx.ext.autodoc.typehints?)
  • It does support TypeAlias to some extent
  • Gets the documentation from import, so we need to get full editable install working which should be done with feat: Switch to importlib.resources #308 for the developer's sake

So far the most promising seems sphinx.ext.autodoc with a design of including the api documentation as .rst files ². Basically to migrate to sphinx.ext.autodoc we need to create the rst document like:

Python-API
=========

.. automodule:: spglib.spglib
   :members:

My initial impressions when using it are good, but we need to work more on both the docstrings and the configuration to make it look good instead of:

Cell documentation

get_symmetry documentation

Footnotes

1. https://github.com/readthedocs/sphinx-autoapi/issues/287 ↩

2. https://github.com/scikit-build/scikit-build-core/blob/main/docs/api/scikit_build_core.build.rst?plain=1 ↩

[LecrisUT]

Also, for this PR, it would be in a 2.4 version, so let's take our time working on this. I will work on #415, so let's not merge anything related to 2.4 until then (with the exception of CI stuff like #417)

[lan496]

I would like to use myst as much as possible for its usability. Also, it is painful to use sphinx.ext.autodoc without editable install. How about using autodoc2 and autodoc2_annotations = False for now, and reconsider when #308 is merged?

[LecrisUT]

How about using autodoc2 and autodoc2_annotations = False for now, and reconsider when #308 is merged?

With the type hints in source? Yeah, that will be good too

[LecrisUT]

I've just added a minor change to scope the typing-extensions dependency, otherwise feel free to rebase and resolve the conflicts.