validation rule for sign of EM fields #6

MasanariHosokawa · 2025-06-16T14:21:06Z

This PR is for addin cocos validation rule in IMAS-Validator. The rule checks AoS time_slice[itime].profiles_2d[i1] and compare the COCOS computed and IDS_COCOS which is from DD. While found there was an issue for reporting in IMAS validator, the current reporting in txt format works and does not block this PR (HTML reporting does not work).

olivhoenen · 2025-06-17T07:47:37Z

imas_validator/assets/rulesets/scenarios/equilibrium.py

+                and profiles_2d.psi.has_value
+                and profiles_2d.r.has_value
+            ):
+                continue


So it will not verify (or can't do it) is any of these quantities is missing in the equilibrium?

No, It will not.

Please note that the rule function will skip calling the compute_COCOS function if any required fields are missing, without reporting an error. @olivhoenen , do you think an error should be reported?

No, an error shall not be reported in case one of these fields is missing.
But we should be able to check COCOS compliancy with DD major version without being mandatory to have ALL these fields, or have it mandatory to have a 2D profiles on a rectangular grid and profiles_2d.grid_type.index == 1 (what is the point of having 33 types of grids -- agreed it's probably too many ;-) -- if we use a single one?)

olivhoenen · 2025-06-17T07:55:25Z

imas_validator/assets/rulesets/scenarios/equilibrium.py

+    """
+    Validate that COCOS computed corresponds to the one in DD.
+    """
+    from idstools.cocos import IDS_COCOS, compute_COCOS


Can we avoid this dependency on idstools? It's not yet open access.

Also I don't understand why the rule is added into the scenarios rulesets, IMO it shall be inside the generic rulesets.

Yes, we can. The cocos.py file has been available in the folder common, and the rule validate_cocos was moved to assets/rulesets/generic.

I don't like it much for cocos.py to be in common, as this is part of the rulesets it should better be next to the rules.

olivhoenen · 2025-06-18T07:56:40Z

imas_validator/common/cocos.py

+import imas
+
+# set cocos in the DD version from the environment
+IDS_COCOS = int(imas.dd_zip.dd_etree().find("cocos").text)


Is this correct? It needs to get the COCOS of the DD version associated with the data-entry being read. I fear that this is going to give the latest no?

Thank you for your comment. The latest commit validates with the DD used to write the IDS.

Copilot

Pull Request Overview

This PR adds a new validation rule for comparing computed COCOS values against those provided by DD in the IMAS-Validator, while also updating a related comment in an existing scenario rule.

Updated comment in scenarios/equilibrium.py to clarify the accessed attribute.
Introduced a new generic rule in generic/equilibrium.py to validate COCOS computation.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
imas_validator/assets/rulesets/scenarios/equilibrium.py	Updated comment to reference global_quantities rather than only ip
imas_validator/assets/rulesets/generic/equilibrium.py	Added validate_cocos function to compare computed COCOS with DD values

Comments suppressed due to low confidence (1)

imas_validator/assets/rulesets/scenarios/equilibrium.py:11

The updated comment does not reflect that the subsequent assertion still checks the 'ip' attribute. Consider updating the comment to clarify that 'ip' is being validated.

        # time_slice[:].global_quantities

imas_validator/assets/rulesets/generic/equilibrium.py

MasanariHosokawa · 2025-06-20T14:07:27Z

There are some errors on the stages "Actions" > "Test using pytest" and "verify-sphinx-doc-generation" as shown below.

It seems the tests do not run with IMAS-Python?

olivhoenen · 2025-06-20T14:13:35Z

There are some errors on the stages "Actions" > "Test using pytest" and "verify-sphinx-doc-generation" as shown below. It seems the tests do not run with IMAS-Python?

No the imas_core mesage, while red and marked as critical is just a warning and is not the problem here (I believe the tests as not relying on imas_core, otherwise if would have never worked on Github as this is not yet publicly available)

The real problem is in validate_cocos:

=========================== short test summary info ============================
FAILED tests/integration/test_generic_ruleset.py::test_generic_tests_with_randomly_generated_ids[equilibrium] - assert (TypeError("unhashable type: 'IDSWrapper'") is None or 'operands could not be broadcast together' in "unhashable type: 'IDSWrapper'")
 +  where TypeError("unhashable type: 'IDSWrapper'") = IDSValidationResult(success=False, msg='', rule=<imas_validator.rules.data.IDSValidationRule object at 0x7f695caac850>...ets/generic/equilibrium.py, line 15 in validate_cocos>], nodes_dict={}, exc=TypeError("unhashable type: 'IDSWrapper'")).exc
 +  and   "unhashable type: 'IDSWrapper'" = str(TypeError("unhashable type: 'IDSWrapper'"))
 +    where TypeError("unhashable type: 'IDSWrapper'") = IDSValidationResult(success=False, msg='', rule=<imas_validator.rules.data.IDSValidationRule object at 0x7f695caac850>...ets/generic/equilibrium.py, line 15 in validate_cocos>], nodes_dict={}, exc=TypeError("unhashable type: 'IDSWrapper'")).exc
================== 1 failed, 168 passed, 2 skipped in 28.70s ===================

MasanariHosokawa · 2025-06-20T16:29:26Z

I'd like to propose to change the name of module from COCOS to ÌMAS_COCOS due to the same name for class and vars. Hapy to have any suggestion.

addapting flake8 fixing format error type annotations in cocos.py mypy style requirements for coding isort cocos validation with version_put in IDS/equilibrium fixing for a CI error fixing the docstring fixing the docstring fixing the docstring fixing the docstring fix on flake8 Update imas_validator/assets/rulesets/generic/equilibrium.py Following Copilot suggestion Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

MasanariHosokawa · 2025-06-23T09:18:41Z

The cocos.py file was reviewed for deploying the lines in the rule function, and I think calling the compute_COCOS module within the validation rule is preferable to embedding the algorithm directly in the rule function.

olivhoenen · 2025-06-23T13:43:14Z

The cocos.py file was reviewed for deploying the lines in the rule function, and I think calling the compute_COCOS module within the validation rule is preferable to embedding the algorithm directly in the rule function.

Approach with compute_COCOS makes it harder to track down the source of the issue, because the assert is on COCOS value, not a single field check. You can try with the following pulse which fails:
imas_validator validate imas:hdf5?path=/work/imas/shared/imasdb/ITER/3/130030/1 -f validate_cocos --debug --verbose

Ideally the validator information shall help finding the source of the issue.

SimonPinches · 2025-06-23T16:15:15Z

I repeat what I said in one of our development meetings, I think we should be checking the consistency of the filled data with our definitions, not trying to compute the COCOS number and seeing if it matches. E.g. In DD/4: sign(poloidal flux) == sign(Ip) and sign(toroidal flux) == sign(B_phi), whilst in DD/3: sign(poloidal flux) == -sign(Ip) and sign(toroidal flux) == sign(B_phi). If we can have separate validation rules for DD/3 and DD/4, we can use our existing approach and don't need to introduce any COCOS machinery.

maarten-ic · 2025-06-24T08:24:22Z

If we can have separate validation rules for DD/3 and DD/4

Note that you can limit validation rules to certain DD versions, see https://imas-validator.readthedocs.io/en/stable/courses/basic/write.html#exercise-6

MasanariHosokawa · 2025-06-26T07:47:49Z

I hope the new commit 4eae751 improves and meets with the reviewer's comments. Regarding the check on the sign of poloidal flux function, I think it is not general enough since Psi is arbitrary to an additive constant which may be chosen for convenience (e.g. for B.C.). @maarten-ic , thank you for your advice telling the way to have rule functions for different DDs.

SimonPinches · 2025-06-26T17:02:38Z

I hope the new commit 4eae751 improves and meets with the reviewer's comments. Regarding the check on the sign of poloidal flux function, I think it is not general enough since Psi is arbitrary to an additive constant which may be chosen for convenience (e.g. for B.C.). @maarten-ic , thank you for your advice telling the way to have rule functions for different DDs.

The DD/4 definition of poloidal flux is not arbitrary up to the addition of a contant. It has an exact definition in terms of integrating the magnetic field through a loop encircling the axis of symmetry (R=0).

SimonPinches · 2025-06-27T08:58:20Z

imas_validator/assets/rulesets/generic/equilibrium.py

+            assert np.sign(time_slice.profiles_1d.f) == b0sign
+
+        if time_slice.profiles_1d.phi.has_value:
+            # phi at center found as zero in DINA, CORSICA and etc.


The toroidal flux at the magnetic axis is zero by definition. We should therefore remove this comment and test that the toroidal flux is zero at the magnetic axis and has the same sign as b0sign at larger radii.
Since this rule does not depend on DD version, can we avoid having to code it up twice and have a generic set of equilibrium validation rules?

SimonPinches · 2025-06-27T09:40:18Z

imas_validator/assets/rulesets/generic/equilibrium.py

+            assert np.sign(psi_boundary - psi_axis) == ipsign
+
+        if time_slice.global_quantities.q_95.has_value:
+            assert np.sign(time_slice.global_quantities.q_95) == ipsign * b0sign


Formally this should be b0sign / ipsign but since we're only dealing with signs, I guess what's written here works.

SimonPinches · 2025-06-27T09:40:22Z

imas_validator/assets/rulesets/generic/equilibrium.py

+            assert np.sign(time_slice.profiles_1d.f) == b0sign
+
+        if time_slice.profiles_1d.phi.has_value:
+            # phi at center found as zero in DINA, CORSICA and etc.


See comment above.

SimonPinches · 2025-06-27T09:41:00Z

imas_validator/assets/rulesets/generic/equilibrium.py

+            assert np.sign(psi_boundary - psi_axis) == -ipsign
+
+        if time_slice.global_quantities.q_95.has_value:
+            assert np.sign(time_slice.global_quantities.q_95) == ipsign * b0sign


See comment above.

SimonPinches · 2025-06-27T09:47:46Z

imas_validator/assets/rulesets/generic/equilibrium.py

+        #
+
+        if time_slice.profiles_1d.f.has_value:
+            assert np.sign(time_slice.profiles_1d.f) == b0sign


Why no tests for f_df_dpsi, pressure , and dpressure_dpsi ?

pressure should be greater than, or equal to, zero, whilst the derivatives would have to be calculated from f and p an approximate comparison made. (Is this possible?)

The rules for f, pressure and dpressure_dpsi have been added.

Would you please tell how f_df_dpsi could be validated?

FYI, multiple cases from DINA-IMAS, CORSICA and etc fail for the rule on dpressure_dpsi.

Since p, f and psi are know, the relevant derivatives could be calculated and and compared within some tolerance with dpressure_dpsi and f_df_dpsi. However, maybe this is now going to far and we should skip such testing for the moment and reconsider later if it's really needed.

I agree, calculation of derived quantities is out of scope here (of possible interest: (https://github.com/ProjectTorreyPines/IMASdd.jl#expressions)), let's get the simple cases in good shape and available rather than trying to be exhaustive.

SimonPinches · 2025-06-27T09:49:17Z

imas_validator/assets/rulesets/generic/equilibrium.py

+        psi_axis = time_slice.global_quantities.psi_axis
+        psi_boundary = time_slice.global_quantities.psi_boundary
+        if psi_axis.has_value and psi_boundary.has_value:
+            assert np.sign(psi_boundary - psi_axis) == -ipsign


I think this is the only line that differs from the DD v3 validation rules above so maybe we can find a way to cut down on duplication / maintenance?

You could have the two validation rules as current, and make a third function that does all of the checking. E.g.

@validator("equilibrium", version="<4.0.0") def validate_sign_DD3(ids): validate_sign_common(ids, is_dd3=True) @validator("equilibrium", version=">=4.0.0") def validate_sign_DD4(ids): validate_sign_common(ids, is_dd3=False) def validate_sign_common(ids, is_dd3): ... # put all validation here, you can use the `is_dd3` boolean to differentiate where needed

This should work, but let me know if you encounter difficulties 🙂

Thank you for your advises about this point. Please check 1637221.

MasanariHosokawa · 2025-07-02T07:42:16Z

I hope the new commit 4eae751 improves and meets with the reviewer's comments. Regarding the check on the sign of poloidal flux function, I think it is not general enough since Psi is arbitrary to an additive constant which may be chosen for convenience (e.g. for B.C.). @maarten-ic , thank you for your advice telling the way to have rule functions for different DDs.

The DD/4 definition of poloidal flux is not arbitrary up to the addition of a contant. It has an exact definition in terms of integrating the magnetic field through a loop encircling the axis of symmetry (R=0).

The rule has been drafted in 1637221 although, found the IDS/equilibrium of multiple METIS cases fails for this rule. For those cases, both psi_axis and psi_boundary are negative where psi_axis > psi_boundary.

olivhoenen · 2025-07-03T07:45:54Z

imas_validator/assets/rulesets/generic/equilibrium.py

+
+
+@validator("equilibrium", version="<4.0.0")
+def validate_sign_DD3(ids):


Can we avoid having a rule that contains the DD version in its name? User should not have to choose which one to apply. Plus the code is redundant except for the last two lines in the assert.

Can we avoid having a rule that contains the DD version in its name? User should not have to choose which one to apply.

Those names are for internal use of IMAS validator which automatically selects the functions based on the DD used in the ids in its design, not for user interface as far as I understand.

Plus the code is redundant except for the last two lines in the assert.

The redundancy has been taken for not using cocos.py and following the tech advice from @maarten-ic. It is possible to recover e.g. #14-15 of the previous commit although, we need some cocos properties more or less in my view when processing multiple DD versions to validate EM data with one function. Is it ok to import cocos.py or any other idea (copying some lines from the module to equilibrium.py)?

MasanariHosokawa · 2025-07-04T13:32:48Z

1e8f2fd merges two functions into one which uses minimum number of cocos properties. Also, I'd like to propose to skip the rule for psi_axis to check if it is positive or negative since the rule practically does not validate existing cases of fixed boundary equilibrium in the database ITER_SCENARIOS.

SimonPinches · 2025-07-04T15:42:41Z

1e8f2fd merges two functions into one which uses minimum number of cocos properties. Also, I'd like to propose to skip the rule for psi_axis to check if it is positive or negative since the rule practically does not validate existing cases of free boundary equilibrium in the database ITER_SCENARIOS.

I'm not sure I fully understand your comment but since we generally test that psi in the plasma has the same sign as Ip for DD/4 and -Ip for DD/3, then this should also apply to psi_axis and psi_boundary.

MasanariHosokawa · 2025-07-09T11:49:44Z

Following Olivier's comment, the two rule functions have been merged into one function and adding docstring for the function ip_b0_sign.

MasanariHosokawa added 3 commits June 16, 2025 15:41

cocos validation rule

29fd9db

linting

5e2a67b

black

49e2f02

MasanariHosokawa requested review from SimonPinches and olivhoenen June 17, 2025 06:54

olivhoenen reviewed Jun 17, 2025

View reviewed changes

MasanariHosokawa added 2 commits June 17, 2025 16:26

PR#6: updates for review comments

89203e3

adding files

404c17f

olivhoenen reviewed Jun 18, 2025

View reviewed changes

SimonPinches requested a review from Copilot June 20, 2025 13:25

Copilot AI reviewed Jun 20, 2025

View reviewed changes

imas_validator/assets/rulesets/generic/equilibrium.py Outdated Show resolved Hide resolved

MasanariHosokawa force-pushed the develop branch from 8b3b014 to 92aba5f Compare June 23, 2025 08:39

MasanariHosokawa changed the title ~~cocos validation rule~~ validation rule for sign of EM fields Jun 26, 2025

MasanariHosokawa added 2 commits June 26, 2025 09:30

validation rule for sign of EM fields

4eae751

improve code format

3781e79

SimonPinches reviewed Jun 27, 2025

View reviewed changes

changes for review's comments

1637221

removing the rule for dpressure_dpsi

863e590

olivhoenen reviewed Jul 3, 2025

View reviewed changes

single rule function for multiple DDs, removing a rule for psi_axis

1e8f2fd

merging two rule functions to one for reviewer's comment

89d2ea2



		@validator("equilibrium", version="<4.0.0")
		def validate_sign_DD3(ids):

validation rule for sign of EM fields #6

Are you sure you want to change the base?

validation rule for sign of EM fields #6

Uh oh!

Conversation

MasanariHosokawa commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

MasanariHosokawa commented Jun 20, 2025

Uh oh!

olivhoenen commented Jun 20, 2025

Uh oh!

MasanariHosokawa commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MasanariHosokawa commented Jun 23, 2025

Uh oh!

olivhoenen commented Jun 23, 2025

Uh oh!

SimonPinches commented Jun 23, 2025

Uh oh!

maarten-ic commented Jun 24, 2025

Uh oh!

MasanariHosokawa commented Jun 26, 2025

Uh oh!

SimonPinches commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MasanariHosokawa commented Jul 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MasanariHosokawa commented Jun 20, 2025 •

edited

Loading

MasanariHosokawa commented Jul 4, 2025 •

edited

Loading

SimonPinches commented Jul 4, 2025 •

edited

Loading