8000 Convert constraints.f90 to Python by timothy-nunn · Pull Request #3630 · ukaea/PROCESS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Convert constraints.f90 to Python #3630

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
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Convert constraints.f90 to Python #3630

wants to merge 10 commits into from

Conversation

timothy-nunn
Copy link
Contributor
@timothy-nunn timothy-nunn commented Apr 11, 2025 •
edited
Loading

Description

Converts constraint_equations.f90 to Python.

Notes

  • I have decided not to convert the additional debugging functions. These can be done by users as required. I only implemented NaN/inf checking on the cc

    PROCESS/source/fortran/constraint_equations.f90

    Lines 311 to 325 in 590cbb6

    8000
    if (isnan(cc(i)).or.(abs(cc(i))>9.99D99)) then
    ! Add debugging lines as appropriate...
    select case (icc(i))
    ! Relationship between beta, temperature (keV) and density (consistency equation)
    case (1); call constraint_err_001()
    ! Equation for net electric power lower limit
    case (16); call constraint_err_016()
    ! Equation for injection power upper limit
    case (30); call constraint_err_030()
    ! Limit on rate of change of energy in poloidal field
    case (66); call constraint_err_066()
    end select
  • I could not/did not want to replicate this weird logic of constraint 15

    PROCESS/source/fortran/constraint_equations.f90

    Lines 981 to 985 in 302cff7

    if (fl_h_threshold > 1.0D0) then
    tmp_symbol = '>'
    else
    tmp_symbol = '<'
    end if
    so I have signed it as >= because it follows the form of other equations with that sign.

@codecov-commenter
Copy link
codecov-commenter commented Apr 11, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 52.16638% with 276 lines in your changes missing coverage. Please review.

Project coverage is 37.67%. Comparing base (b9cb2bc) to head (e0264d3).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
process/constraints.py 51.85% 273 Missing ⚠️
process/final.py 33.33% 2 Missing ⚠️
process/caller.py 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3630      +/-   ##
==========================================
+ Coverage   36.28%   37.67%   +1.38%     
==========================================
  Files          88       89       +1     
  Lines       22237    23554    +1317     
==========================================
+ Hits         8069     8873     +804     
- Misses      14168    14681     +513

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timothy-nunn timothy-nunn force-pushed the convert-constraints-f90 branch 2 times, most recently from a7c88e6 to 22397e2 Compare April 11, 2025 15:45
@timothy-nunn timothy-nunn self-assigned this Apr 14, 2025
@timothy-nunn 8000 timothy-nunn force-pushed the convert-constraints-f90 branch 5 times, most recently from d7a917f to 1842ac5 Compare April 15, 2025 15:21
@timothy-nunn timothy-nunn force-pushed the convert-constraints-f90 branch 7 times, most recently from ed63438 to 6317dfe Compare April 23, 2025 16:02
@timothy-nunn timothy-nunn marked this pull request as ready for review April 23, 2025 16:09
@timothy-nunn timothy-nunn requested a review from jonmaddock April 23, 2025 16:10
@timothy-nunn timothy-nunn force-pushed the convert-constraints-f90 branch from 8d5fe79 to b281443 Compare April 24, 2025 09:13
@timothy-nunn timothy-nunn linked an issue Apr 24, 2025 that may be closed by this pull request
Convert constraint_equations.f90 to Python #3431
Open
@timothy-nunn timothy-nunn force-pushed the convert-constraints-f90 branch 2 times, most recently from 406e808 to dce76f6 Compare April 30, 2025 09:26
@timothy-nunn timothy-nunn mentioned this pull request Apr 30, 2025
Draft
jonmaddock
jonmaddock requested changes May 1, 2025
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.

This is a great step, but I'm not totally convinced about the additional complexity of the decorator-based registration idea. What's the advantage of this approach?

jonmaddock
jonmaddock requested changes May 12, 2025
View reviewed changes
Copy link
< 8000 /details>
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.

Thanks for explaining your approach, I'm happy with this now. Could you add docstrings to your new classes please, and find if there are any "adding a new constraint" docs which are now out of date. Could you deal with my minor "returning an array" comment, and lastly change cc and others to something more descriptive?

@timothy-nunn timothy-nunn force-pushed the convert-constraints-f90 branch from 858bcd2 to a3604e1 Compare May 16, 2025 10:21
@timothy-nunn timothy-nunn force-pushed the convert-constraints-f90 branch from 41697fb to e0264d3 Compare May 16, 2025 13:11
@timothy-nunn timothy-nunn requested a review from jonmaddock May 16, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonmaddock jonmaddock Awaiting requested review from jonmaddock

Requested changes must be addressed to merge this pull request.

Assignees

@timothy-nunn timothy-nunn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert constraint_equations.f90 to Python
3 participants
@timothy-nunn @codecov-commenter @jonmaddock
0