8000 Collate cost models by je-cook · Pull Request #3045 · ukaea/PROCESS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Collate cost models #3045

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
Merged
merged 5 commits into from
Feb 8, 2024
Merged

Collate cost models #3045

merged 5 commits into from
Feb 8, 2024

Conversation

je-cook
Copy link
Collaborator
@je-cook je-cook commented Feb 2, 2024

Description

Collates cost models into one interface and does a bit of cleanup while I was there

Checklist

I confirm that I have completed the following checks:

  • 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.

@je-cook je-cook added the Cost model Relating to cost models label Feb 2, 2024
@je-cook
Copy link
Collaborator Author
je-cook commented Feb 2, 2024

@j-a-foster if you want to do anymore removals here feel free. I dunno how access works on forks but let me know if you have problems and i'll fix it

@je-cook
Copy link
Collaborator Author
je-cook commented Feb 5, 2024

As far as I can work out its not possible to inject the custom cost model while using either the main function or the Process class (therefore from the cli).

Is there any preference about how this could be done/if it needs to be done?
Current options as I see it if something is done to specify the python class to import:

  1. add another command line arg
  2. add a variable to the inputfile
  3. leave it as is and specification of a custom model requires injection through python (and initialisation of either VaryRun or SingleRun.

For point 3 my last commit unifies the running interface between VaryRun and SingleRun.

Let me know thoughts and I can modify as needed.

@je-cook je-cook force-pushed the collate_cost_models branch from 202728f to 37a0525 Compare February 5, 2024 08:39
@je-cook je-cook force-pushed the collate_cost_models branch from 37a0525 to 90b809d Compare February 5, 2024 08:50
@jonmaddock
Copy link
Contributor
jonmaddock commented Feb 5, 2024 •
edited
Loading

Thanks for this James. @timothy-nunn can you review this please? Thanks.

@jonmaddock jonmaddock assigned timothy-nunn and unassigned jonmaddock Feb 5, 2024
timothy-nunn
timothy-nunn approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor
@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.

Confirmed this does not change the running of SingleRun or VaryRun or change any of the large-tokamak values.

@timothy-nunn
Copy link
Contributor

@j-a-foster have you used this interface yet, and do you have any insights on how easy it is to use custom cost models?

@j-a-foster
Copy link
Collaborator

@timothy-nunn I've created an MR in the cost model repo that reflects the changes made here and checked that it works. Currently the only example is in the private cost model repo but we could create an example in this repo of how a user can set up their own custom cost model to work with PROCESS.

@timothy-nunn timothy-nunn merged commit 8e12bdd into ukaea:main Feb 8, 2024
@je-cook je-cook deleted the collate_cost_models branch June 5, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothy-nunn timothy-nunn timothy-nunn approved these changes

Assignees

@timothy-nunn timothy-nunn

Labels
Cost model Relating to cost models
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@je-cook @jonmaddock @timothy-nunn @j-a-foster
0