Raise TypeError for invalid cell#420
Conversation
|
The current CI does not check on ubuntu-latest. That should be fixed. |
|
@LecrisUT run-cmake on CI starts to fail. Can you take a look? |
Would need to use something like https://github.com/rscohn2/setup-oneapi. Not sure how well that action works. |
43027d2 to
70fe209
Compare
|
I realize the annotation is not useful in keeping the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
LecrisUT
left a comment
There was a problem hiding this comment.
We should still add the cell type-hinting because from the typeerror messages it is unclear which position is which that raised the error
|
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. |
70fe209 to
790b63c
Compare
What you had before was a good backwards compatible approach, you just needed to add those into |
In this case, autodoc2 just shows |
Yes, that is the intended output. What we should check is how to add those aliases to the documentation, either via |
|
autodoc2_replace_annotations will be the way for it. But I still do not expect it helps users. |
Can you remove the |
|
After removing |
|
@LecrisUT 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.
Now I think the option 2 is tolerable. What do you think? |
|
@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
So far the most promising seems 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: Footnotes |
|
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 |
With the type hints in source? Yeah, that will be good too |
1ded420 to
65df729
Compare
95aedaf to
d2f6246
Compare
Signed-off-by: Cristian Le <[email protected]>
Trying to revisit #420 In principle we can use `autodoc_type_aliases` to simplify the documentation


Closes #412 and #411
Add naive type annotation forcellTypeErrorwhencelldoes not have appropriate shapes