Skip to content

Stellarator module update#4082

Open
jjwalkowiak wants to merge 81 commits intoukaea:mainfrom
IPP-SRS:main
Open

Stellarator module update#4082
jjwalkowiak wants to merge 81 commits intoukaea:mainfrom
IPP-SRS:main

Conversation

@jjwalkowiak
Copy link

Description

Main changes:

  • coil minor radius scaling for stellarators
  • corrected stress equation for vacuum vessel
  • corrected coil height (this was actually more on the pre-process side)
  • stellarator module refactoring

Checklist

I confirm that I have completed the following checks:

  • My changes follow the PROCESS style guide
  • I have justified any large differences in the regression tests caused by this pull request in the comments.
  • I have added new tests where appropriate for the changes I have made.
  • If I have had to change any existing unit or integration tests, I have justified this change in the pull request comments.
  • If I have made documentation changes, I have checked they render correctly.
  • I have added documentation for my change, if appropriate.

jjwalkowiak and others added 30 commits March 7, 2025 16:45
Copy link
Collaborator

@timothy-nunn timothy-nunn left a comment

Choose a reason for hiding this comment

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

The pre-commit quality jobs are failing again. You can install pre-commit as a hook by following our guide here https://ukaea.github.io/PROCESS/development/pre-commit/. This will flag any non-compliant code and automatically update it to follow the style guide (in most cases)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am happy with these changes to the unit test file provided you and your team are happy that the numbers have changed for a legitimate reason.

@timothy-nunn
Copy link
Collaborator

Who created test: tests/unit/test_stellarator.py::test_stbild ? Either I misunderstand something, or the reference values are contradicting each other. f_st_rmajor=0.99099099099099097, f_st_aspect=1, f_st_rminor=0.99125889880147788, How can it be, that minor and major radius have different scaling, but aspect ration is not changed?

I believe this was an automatic test. When we converted PROCESS from Fortran to Python, we ran regression tests and scraped data from functions we were converting. This allowed us to test that we hadn't changed the codes' behaviour between Fortran and Python

@timothy-nunn
Copy link
Collaborator

Please also ensure the unit test failures are fixed and that the regression tests stop failing with an error

I solved most issues with units test, but with regression tests it will be more complicated.

Just to clarify that your regression tests don't need to pass, there are several failure mechanisms that a regression test my experience:

  • An error occurs when the input file is run (this is what is happening on this branch). This type of test failure is not ok and must be fixed.
  • An error is raised because the MFile is different from the reference MFile. This is ok, as long as you and the reviewers are happy with the changes. When the PR is merged, it will update the reference MFile.

@jjwalkowiak
Copy link
Author

Please also ensure the unit test failures are fixed and that the regression tests stop failing with an error

I solved most issues with units test, but with regression tests it will be more complicated.

Just to clarify that your regression tests don't need to pass, there are several failure mechanisms that a regression test my experience:

* An error occurs when the input file is run (this is what is happening on this branch). This type of test failure is not ok and must be fixed.

* An error is raised because the MFile is different from the reference MFile. This is ok, as long as you and the reviewers are happy with the changes. When the PR is merged, it will update the reference MFile.

I think it would be best to create new regression test (input+stella_config). I will deal with this after my vacation.

@timothy-nunn
Copy link
Collaborator

Please also ensure the unit test failures are fixed and that the regression tests stop failing with an error

I solved most issues with units test, but with regression tests it will be more complicated.

Just to clarify that your regression tests don't need to pass, there are several failure mechanisms that a regression test my experience:

* An error occurs when the input file is run (this is what is happening on this branch). This type of test failure is not ok and must be fixed.

* An error is raised because the MFile is different from the reference MFile. This is ok, as long as you and the reviewers are happy with the changes. When the PR is merged, it will update the reference MFile.

I think it would be best to create new regression test (input+stella_config). I will deal with this after my vacation.

Hey @jjwalkowiak happy with this solution... as long as the stellarator is well tested I am happy for files to be updated or for new ones to be made.

@jjwalkowiak
Copy link
Author

I updated stellarator input file for regression test. stella_config seems to work. On my machine I get: "4626 differences: the reference MFile contains different values for some of the variables. See the warnings for a breakdown of the differences.".
So I think it works as expected. I just didn't found the reference MFile.

@timothy-nunn
Copy link
Collaborator

@jjwalkowiak Could you please fix the merge conflicts so that I can run the tests and then hopefully we are not too far off this being ready.

@jjwalkowiak
Copy link
Author

jjwalkowiak commented Feb 17, 2026 via email

@timothy-nunn
Copy link
Collaborator

Hi @jjwalkowiak

There is still an error for one of the Stellarator test files:

FAILED tests/regression/test_process_input_files.py::test_input_file[helias_5b] - RuntimeError:  An error occured while running PROCESS: float division by zero

@jjwalkowiak
Copy link
Author

jjwalkowiak commented Feb 18, 2026 via email

@jjwalkowiak
Copy link
Author

jjwalkowiak commented Feb 18, 2026 via email

@jjwalkowiak
Copy link
Author

Ok, now it should work (hopefully)

Copy link
Collaborator

@timothy-nunn timothy-nunn left a comment

Choose a reason for hiding this comment

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

Two very minor things and then I am happy once @ym1906 has had a quick look

@jjwalkowiak
Copy link
Author

Done. Sorry for this revert madness, I am just too lazy to do this correctly :p

Copy link
Collaborator

@timothy-nunn timothy-nunn left a comment

Choose a reason for hiding this comment

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

Happy with the code and changes. Will approve once @ym1906 has approved

Copy link
Collaborator

@ym1906 ym1906 left a comment

Choose a reason for hiding this comment

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

These changes look good to me. There's nothing alarming in the regression tests, and further to previous comments there are not significant modelling changes.

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.

4 participants

Comments