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

Minor docstring formatting fixes, refactor input argument validation, and fix bug when tracing in +/-1 direction #172

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

wtbarnes
Copy link
Member

@wtbarnes wtbarnes commented Oct 3, 2024

This PR does two three things:

  • Fix some of the formatting in the docstrings for VectorGrid and Streamtracer. Also moved xs and ROT to be properties so that their attribute docstrings render more nicely.
  • Refactor the input validation on VectorGrid. Previously, the validation was done using a mix of some setters and some static validation methods. This refactor moves all of the validation to setter methods.
  • Fixes a bug in Streamtracer.trace() in the direction=+/-1 cases where the streamlines were trying to be returned as an array, but were failing because each array within that array was of unequal length. This returns them as a list, consistent with the direction=0 case. Maybe this needs a test? If it was already covered, I'm not sure how it was passing previously. I could move this to a separate PR if that is preferable.

nabobalis
nabobalis previously approved these changes Oct 3, 2024
@wtbarnes wtbarnes changed the title Minor docstring formatting fixes and refactor input argument validation Minor docstring formatting fixes, refactor input argument validation, and fix bug when tracing in +/-1 direction Oct 3, 2024
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Looks good, just missing a couple of docstrings on properties.

python/streamtracer/streamline.py Show resolved Hide resolved
python/streamtracer/streamline.py Show resolved Hide resolved
@wtbarnes
Copy link
Member Author

I've added docstrings for all the new properties. I'm not sure why the RTD build is failing...

@nabobalis
Copy link
Contributor

Just needs a kick.

@nabobalis nabobalis requested a review from Cadair October 16, 2024 03:32
python/streamtracer/streamline.py Outdated Show resolved Hide resolved
python/streamtracer/streamline.py Outdated Show resolved Hide resolved
python/streamtracer/streamline.py Outdated Show resolved Hide resolved
@Cadair Cadair enabled auto-merge (squash) October 16, 2024 13:28
@Cadair Cadair merged commit 315e4bb into sunpy:main Oct 16, 2024
14 checks passed
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.

3 participants