-
Notifications
You must be signed in to change notification settings - Fork 104
Validator for profile_conditions #1630
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.
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
base: main
Are you sure you want to change the base?
Conversation
c15a2b4 to
1e93d3b
Compare
| import torax._src.constants as constants | ||
|
|
||
| _PROFILE_CONDITIONS_REQUIRED_FIELDS = { | ||
| "gloabl_quantities": ["ip", "v_loop"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any constants from the imas-python library that can be referred to here instead of us using literal strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikesndrs can I ask your input on this ? Do you know if it exists in imas-python ?
| for field in _PROFILE_CONDITIONS_REQUIRED_FIELDS["gloabl_quantities"]: | ||
| if not getattr(global_quantities, field): | ||
| # Warning or critical ? | ||
| logging.critical( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we'd want to raise an error here with an informative error message that the expected field is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure about this. For geometry loader I think we'd want to raise an error but with core_profiles not necessarily. There might be cases where we want to load partially some profiles but not all from an IDS, like only electrons ones or ions ones and get the rest from elsewhere / prescribe it. In this case, it does not matter that the other fields are empty in the input IDS. Do you agree ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, yeah that makes sense. I think it would be good to converge on a minimal set of required attributes for using the profile_conditions_from_IMAS function though as that makes assumptions on certain fields being present?
Validator which should allow validation for plasma_composition and profile_conditions Currently do not check if individual ions density are filled
27b60c5 to
c9b5e80
Compare
Can be empty when the ids contains only a single time slice, as in step_flattop_bg example
|
Hey @Nush395, if you can have a look at this version to tell me if this goes in the right direction or not. Still needs some changes but just to know if it's worth making the changes or changing the structure.
Maybe it would be better to directly pass the validation_fields dict as an arg of the function instead of the current "target"? This way it would allow to be user defined fields. |
Nush395
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it's actually cleaner to just do this validation explicitly. I've written out an example in #1866 to demonstrate.
I feel that is a slightly more transparent solution as you can directly see in the function what validation is being done and there aren't actually too many fields being validated. wdyt of that approach?
Add validators for the IMAS function to check that the required fields are properly filled in the input IDS and if not provide a clear warning.
Closes #1610.
Validation functions:
Integration