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.

Sign up for GitHub

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

Jump to bottom

Add usage to README and add tests with private data and fix a few bugs #11

Merged
merged 6 commits into from
Jun 10, 2024
Merged

Add usage to README and add tests with private data and fix a few bugs #11

merged 6 commits into from
Jun 10, 2024

Conversation

je-cook
Copy link
Contributor

@je-cook je-cook commented Jun 10, 2024

  • Adds some basic usage data to the README.
  • Pulls over some more tests from bluemira
  • Fixes 3 bugs
    1. old json eqdsks sometimes contain pnorm instead of psinorm
    2. fix eqdsk writing bug
    3. In the strict g-eqdsk standard qpsi must have the same length as nx.
      • This is now enforced in an eqdsk file only
      • A choice should be made about the fill value, if qpsi is a single value we fill with that, if it is multiple we fill with 0 to the length of nx, if qpsi is None we fill with 0

@je-cook je-cook added bug Something isn't working documentation Improvements or additions to documentation CI tests labels Jun 10, 2024
@je-cook je-cook requested a review from a team as a code owner June 10, 2024 06:44
oliverfunk
oliverfunk requested changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@oliverfunk oliverfunk left a comment

Choose a reason for hiding this comment

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

Re. qpsi not being defined in an eqdsk file, or not being the correct length (same length as nx)

  • For qpsi not being defined: it should be set to an array of 1's, length of nx
  • For qpsi not being the correct length: we should raise an error.

@je-cook je-cook requested a review from oliverfunk June 10, 2024 09:48
oliverfunk
oliverfunk reviewed Jun 10, 2024
View reviewed changes
eqdsk/file.py Outdated Show resolved Hide resolved
oliverfunk
oliverfunk reviewed Jun 10, 2024
View reviewed changes
eqdsk/file.py Outdated Show resolved Hide resolved
oliverfunk
oliverfunk reviewed Jun 10, 2024
View reviewed changes
eqdsk/tools.py Outdated Show resolved Hide resolved
oliverfunk
oliverfunk approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@oliverfunk oliverfunk left a comment

Choose a reason for hiding this comment

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

Few nits, otherwise all good!

je-cook and others added 2 commits June 10, 2024 13:41
@je-cook @oliverfunk
Apply suggestions from code review
c97ee65 
Co-authored-by: Oliver Funk <oliverfunk@users.noreply.github.com>
@je-cook
no suggestion
7da9740
@oliverfunk oliverfunk enabled auto-merge (squash) June 10, 2024 12:54
@oliverfunk oliverfunk merged commit e2fb1bb into main Jun 10, 2024
2 checks passed
@oliverfunk oliverfunk deleted the je-cook/fixes_and_tests branch June 10, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverfunk oliverfunk oliverfunk approved these changes

Assignees

@oliverfunk oliverfunk

Labels
bug Something isn't working CI documentation Improvements or additions to documentation tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@je-cook @oliverfunk