Skip to content

Conversation

@jurasic-pf
Copy link
Contributor

@jurasic-pf jurasic-pf commented Nov 26, 2025

A first step in addressing issue #547 of vmec_compute_geometry

image image

@jurasic-pf
Copy link
Contributor Author

CI failiures are due to the known issue with SPEC CI

@mishapadidar mishapadidar self-requested a review December 1, 2025 19:10
@mishapadidar
Copy link
Contributor

We have disabled SPEC in the github actions in PR 571.
I just retriggered the tests

Copy link
Contributor

@mishapadidar mishapadidar left a comment

Choose a reason for hiding this comment

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

This is a really nice PR.
I mostly left small comments. Primarily, I would like to see how the documentation renders the dataclass.
There is a little bit of rewriting left to do in the documentation of the vmec_compute_geometry and VmecGeometryResults now that the attributes will be stored in the dataclass.

@jurasic-pf
Copy link
Contributor Author

It's a bit unfortunate that the constructor takes up so much space in the docs, but other than that everything renders nicely now.

I used Claude to accelerate generating the docstrings, I'm not familiar with all quantities (especially not the gyrokinetic ones), but checked everything related to coordinates and ideal MHD.

@jurasic-pf
Copy link
Contributor Author

If you want to allow dict like access, we can also add

def __getitem__(self, item):
    return getattr(self, item)

but afaik Struct isn't subscriptable either

@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.06%. Comparing base (ab9096f) to head (6241f45).
⚠️ Report is 20 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   84.89%   85.06%   +0.17%     
==========================================
  Files          83       83              
  Lines       16291    16458     +167     
==========================================
+ Hits        13830    14000     +170     
+ Misses       2461     2458       -3     
Flag Coverage Δ
unittests 85.06% <100.00%> (+0.17%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mishapadidar
Copy link
Contributor

I agree, the arguments take up quite a bit of space. It seems like we might be able to prevent sphinx from rendering them. Evidently, sphinx looks for the __signature__ attribute when rendering the docs. Adding

import inspect
VmecGeometryResults.__signature__ = inspect.Signature()

should reduce the signature to VmecGeometryResults(). Although, im not sure if this signature is better since it suggests that there are no arguments to the class. We could also write a custom signature.

@jurasic-pf
Copy link
Contributor Author

I agree, the arguments take up quite a bit of space. It seems like we might be able to prevent sphinx from rendering them. Evidently, sphinx looks for the __signature__ attribute when rendering the docs. Adding

import inspect
VmecGeometryResults.__signature__ = inspect.Signature()

should reduce the signature to VmecGeometryResults(). Although, im not sure if this signature is better since it suggests that there are no arguments to the class. We could also write a custom signature.

Whatever you think is best.
Anyone reading dataclass in code will know what it is, so I think making the signature empty in sphinx docs is acceptable.

Copy link
Contributor

@mishapadidar mishapadidar left a comment

Choose a reason for hiding this comment

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

Since we are planning on omitting the arguments in the Sphinx docs, I have added one more minor comment to help users reading the documentation.
After this, I think we can merge.

@dataclasses.dataclass
class VmecGeometryResults:
"""
A class to hold the output of the :func:`vmec_compute_geometry` and :func:`vmec_fieldlines` functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are no longer using the sphinx signature, perhaps we should add one line to the documentation that discusses this. For example, we could add something like:
The arguments required of this class have been hidden in the Sphinx documentation, since they are extensively long. See the code for vmec_compute_geometry for an example of initializing this class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants