Skip to content

3131 errror in calculation of temperature margin bug#3142

Merged
jonmaddock merged 2 commits intomainfrom
3131-errror-in-calculation-of-temperature-margin-bug
May 13, 2024
Merged

3131 errror in calculation of temperature margin bug#3142
jonmaddock merged 2 commits intomainfrom
3131-errror-in-calculation-of-temperature-margin-bug

Conversation

@mkovari
Copy link
Copy Markdown
Collaborator

@mkovari mkovari commented Apr 17, 2024

Description

I have replaced the home-made Newton-Raphson solver and the secant solver with newton for calculating the temperature margin of superconductors.

Checklist

I confirm that I have completed the following checks:

  • No large differences in the regression tests except for constraint residues.
  • No new tests
  • No doc changes

@mkovari mkovari linked an issue Apr 17, 2024 that may be closed by this pull request
@jonmaddock
Copy link
Copy Markdown
Contributor

Hi Michael. Can you resolve the conflicts on this branch, then possibly tidy up the commit history with an interactive rebase if you feel that would make these changes clearer?

@mkovari
Copy link
Copy Markdown
Collaborator Author

mkovari commented Apr 19, 2024 via email

@mkovari
Copy link
Copy Markdown
Collaborator Author

mkovari commented Apr 19, 2024

Anyway - there are now no merge conflicts.
I don't know what an interactive rebase is.

@mkovari mkovari force-pushed the 3131-errror-in-calculation-of-temperature-margin-bug branch from 42ac072 to 37e5a42 Compare April 19, 2024 10:39
@mkovari
Copy link
Copy Markdown
Collaborator Author

mkovari commented Apr 19, 2024

I believe this is now ready to merge. I did both the things we discussed:

  1. Replaced brentq by newton.
  2. Restored the commented-out integration tests, changed the value of rohc to something more sensible, and changed the two assertions that failed as a result.

@mkovari mkovari marked this pull request as draft April 19, 2024 15:35
@mkovari mkovari marked this pull request as ready for review April 19, 2024 15:55
Copy link
Copy Markdown
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.

Hi Michael, I don't see anything wrong with these changes, other than the large amount of churn on this PR which makes it confusing. Please rebase and fix up these commits to simplify the history, and then I don't see any reason not to merge.

Comment thread process/pfcoil.py Outdated

deltaj_hijc_rebco = jcrit0 - jsc
return deltaj_hijc_rebco
# def deltaj_nbti(temperature):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a lot that's commented out here, and the commit title "incomplete changes to hijc_rebco" suggests that this is an intermediary commit. Can this be fixed up please to reduce the churn in this PR?

Comment thread process/maths_library.py Outdated
Comment thread tests/integration/data/test_solver.conf
Comment thread tests/integration/test_pfcoil_int.py Outdated
# assert pytest.approx(pfv.rjohc) == -7.728453e9


def test_efc(pfcoil: PFCoil[CsFatigue], monkeypatch: pytest.MonkeyPatch):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pfcoil: PFCoil[CsFatigue]: I'm not sure that's the correct type hint.

Comment thread tests/integration/test_pfcoil_int.py Outdated


def test_efc(pfcoil, monkeypatch):
# def test_pfcoil(monkeypatch, pfcoil):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As all this is commented back in later, can this be fixed up to reduce churn in this PR?

Comment thread tests/integration/test_pfcoil_int.py
Comment thread process/pfcoil.py

# assert pytest.approx(pfv.bpf[4]) == 9.299805e2
# assert pytest.approx(pfv.rjohc) == -7.728453e9
def test_pfcoil(monkeypatch, pfcoil):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Commenting back in of commented-out function: please reduce churn.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Will do using rebase

Comment thread process/pfcoil.py Outdated
@mkovari mkovari force-pushed the 3131-errror-in-calculation-of-temperature-margin-bug branch from ec3b934 to cad1ed2 Compare May 2, 2024 13:45
@mkovari mkovari requested a review from jonmaddock May 2, 2024 13:47
@jonmaddock
Copy link
Copy Markdown
Contributor

Can you resolve the conflicts please?

@mkovari
Copy link
Copy Markdown
Collaborator Author

mkovari commented May 9, 2024

I think the conflict is resolved now.

@jonmaddock
Copy link
Copy Markdown
Contributor

I'm afraid there are still conflicts that need to be resolved. Please resolve them by rebasing, then re-request a review. Thanks.

@mkovari
Copy link
Copy Markdown
Collaborator Author

mkovari commented May 13, 2024

GitHub says this branch has no conflicts with the base branch. What conflicts are you thinking of?

@mkovari mkovari force-pushed the 3131-errror-in-calculation-of-temperature-margin-bug branch from f29a035 to 45e0187 Compare May 13, 2024 15:15
@mkovari mkovari requested a review from jonmaddock May 13, 2024 15:18
@jonmaddock jonmaddock merged commit 8f14e99 into main May 13, 2024
@jonmaddock jonmaddock deleted the 3131-errror-in-calculation-of-temperature-margin-bug branch May 13, 2024 15:27
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.

Errror in calculation of temperature margin [BUG]

2 participants