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

Fixed #3150 (winding pack current density constraint) #3160

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixed #3150 (winding pack current density constraint) #3160

wants to merge 1 commit into from
+95 −308

Conversation

mkovari
Copy link
Collaborator

@mkovari mkovari commented Apr 25, 2024

Fixed #3150.
Removed obsolete references to PROCESS Users's Guide.

@mkovari mkovari linked an issue Apr 25, 2024 that may be closed by this pull request
Incorrect error in winding pack current density constraint. #3150
Open
jonmaddock
jonmaddock requested changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@jonmaddock jonmaddock left a comment

Choose a reason for hiding this comment

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

I think there's some indentation that needs correcting. Also, why has an image been put in the examples directory? Perhaps this belongs in the docs?

process/current_drive.py Outdated
@@ -1279,8 +1278,7 @@ def iternb(self):
fshine : output real : shine-through fraction of beam
This routine calculates the current drive parameters for a
neutral beam system, based on the 1990 ITER model.
AEA FUS 251: A User's Guide to the PROCESS Systems Code
ITER Physics Design Guidelines: 1989 [IPDG89], N. A. Uckan et al,
ITER Physics Design Guidelines: 1989 [IPDG89], N. A. Uckan et al,
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation please.

process/evaluators.py Outdated
@@ -37,8 +37,7 @@ def fcnvmc1(self, n, m, xv, ifail):
n-dimensional point of interest xv.
Note that the equality constraints must precede the inequality
constraints in conf.
AEA FUS 251: A User's Guide to the PROCESS Systems Code
:param n: number of variables
:param n: number of variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, fix indentation please.

@mkovari
Copy link
Collaborator Author

mkovari commented Apr 26, 2024 •
edited

The image files plot_proc_1, 2 and 3 must have been put into the examples folder automatically when I ran pytest. Because there were so many modified files I used git commit -a, and because I didn't have the examples folder open in VSCode I didn't see they had been changed.

However, they seem to fit, and since only the new third page is shown in the changes tab, I suppose that the other two pages, plot_proc_1.png and plot_proc_2.png were already there, so I have left plot_proc_3.png there.

@mkovari
Copy link
Collaborator Author

mkovari commented Apr 26, 2024 •
edited

I have corrected indentations in several files.

@mkovari mkovari changed the title Fixed #3150 (winding pack current densoty constraint) Fixed #3150 (winding pack current density constraint) Apr 26, 2024
@mkovari mkovari marked this pull request as draft April 26, 2024 14:38
@mkovari mkovari marked this pull request as ready for review April 26, 2024 15:06
@mkovari mkovari requested a review from jonmaddock April 26, 2024 15:06
jonmaddock
jonmaddock requested changes May 3, 2024
View reviewed changes
Copy link
Contributor

@jonmaddock jonmaddock left a comment

Choose a reason for hiding this comment

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

Please fixup the changes to comments to eliminate the 2 correcting commits and eliminate churn. The plot_procX.png figures should be created by running the examples.ipynb notebook, and the resulting pngs displayed in the notebook. Please look at how figure 1 and 2 are being handled by the notebook to match. Thanks!

@mkovari
Copy link
Collaborator Author

mkovari commented May 7, 2024

The plot_proc figures have nothing to do with this pull request. They result from Chris's edit. If the example notebook needs changing it should be done there. I will delete the unwanted png.

@mkovari mkovari force-pushed the 3150-incorrect-error-in-winding-pack-current-density-constraint branch from bb88e51 to 10814bc Compare May 9, 2024 13:52
@mkovari
Copy link
Collaborator Author

mkovari commented May 9, 2024

Unwanted png removed. Rebased.

@mkovari
Copy link
Collaborator Author

mkovari commented May 14, 2024

I have fixed the conflict localy, but when I rebased and then tried to force push, I couldn't do it:

(env) mkovari@J1714:~/PROCESS$ git push --force
ssh: connect to host github.com port 22: Connection timed out
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@mkovari mkovari force-pushed the 3150-incorrect-error-in-winding-pack-current-density-constraint branch from 10814bc to b1db908 Compare May 14, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonmaddock jonmaddock jonmaddock requested changes

@timothy-nunn timothy-nunn Awaiting requested review from timothy-nunn

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect error in winding pack current density constraint.
2 participants
@mkovari @jonmaddock