Mark namefitted group as undocumented #570

lukaspie · 2025-03-03T10:25:11Z

@rettigl with this PR, we get an "is written without documentation" warning if a variadic concept name is used that is not part of the application definition.

I decided to reuse the undocumented warning here because this can be turned off with a flag in the CLI call of dataconverter. So if somebody is sure that they want to use this namefit anyway, we can just allow that by passing the `ignore-undocumented´ flag.

Fixes #563

…e with documented keys

…tely

rettigl

This change seems to work, however it forces now the NAME[instance] notation also for variadic Fields. In fact, I support this, as previously namefitting was sometimes not correct.

One improvement I suggest is to add a separate clause for NXcollections. These can as I understand it appear anywhere:

"This is the NXcollection class which can appear anywhere underneath NXentry"

src/pynxtools/dataconverter/validation.py

rettigl · 2025-03-03T20:25:11Z

I used NXroot before to write nexus files w/o schema. These give warnings now, see igor-reader tests. Maybe this is fine.

rettigl · 2025-03-03T20:26:47Z

Mpes test failures are all with PROCESS_MPES, which should be removed anyways, so this will be resolved later.

rettigl · 2025-03-03T20:28:32Z

I used NXroot before to write nexus files w/o schema. These give warnings now, see igor-reader tests. Maybe this is fine.

Ah no, NXroot contains NXentry. The issue is that we need ENTRY[scan...]/DATA[data]/DATA[data] now.

Fix for checking of undocumented keys

rettigl · 2025-03-05T08:21:34Z

@lukaspie Maybe we merge this into #554, fix the tests accordingly, and once the new definitions are out, merge this into master? Or do you think it makes sense fixing reader tests now before the new release comes?

Add more tests

…s' into 563-bug-converter-does-not-warn-if-variadic-concept-name-is-invalid

rettigl · 2025-03-14T08:57:11Z

@lukaspie I reset this to merge to master to run tests.
I added a test for a failing namefitting, which namefits any illegal fixed name to a variadic group with required children. Maybe you can have a look how to solve this.

lukaspie · 2025-03-14T16:52:04Z

@rettigl I implemented what we discussed today, i.e. you need to give the concept name if you want to use a variadic concept. Note that we still need to do namefitting because we have the additional constraint of the nameType which the new field needs to fit to (this is not super important here, but will be with the new definitions). I think the new version will make namefitting and testing much cleaner.

Validation tests are passing now, but the plugin tests are still failing. Some of that is related to the addittional checks we are running now (like the type of the @vector attribute in MPES), but there seem to be some things that are additionally wrong:

For pynxtools-mpes, WARNING: Missing attribute: "/ENTRY[entry]/data/@signal". Not sure where that comes from.

Note that for pynxtools-mpes, I get Field /ENTRY[entry]/PROCESS_MPES[process]/CALIBRATION[ky_calibration]/calibrated_axis written without documentation even though NXcalibration/calibrated_axis is a non-variadic concept. This should actually be /ENTRY[entry]/PROCESS_MPES[process]/kN_CALIBRATION[ky_calibration]/calibrated_axis, then it works.

Will continue next week.

rettigl · 2025-03-14T18:52:04Z

Excellent, thanks. I already briefly looked into the mpes errors. The calibration error comes because in the referenced definitions, it is calibrated_AXIS for some reason, instead of calibrated_exis in NXcalibration. Not sure where that came from. The other issue I think is also with the config file, I will start an appdate branch.

rettigl · 2025-03-14T20:49:41Z

Excellent, thanks. I already briefly looked into the mpes errors. The calibration error comes because in the referenced definitions, it is calibrated_AXIS for some reason, instead of calibrated_exis in NXcalibration. Not sure where that came from. The other issue I think is also with the config file, I will start an appdate branch.

Checking of group attributes was not properly handled, because the "@" was not removed in the keys dict. I changed the logic now that the @ should be in the dict for all attributes.

What is still not working now is the proper handling of @url, because it is considered a concept rather than a name. This should be hopefully fixed by the new nameType code, I assume?

lukaspie · 2025-03-17T08:15:15Z

@rettigl why was you last commit (25e0013) neccessary? I think we can just do:

best_match = None

for node in nodes:
    if not node.variadic:
        if instance_name == node.name:
            return node
    else:
        if concept_name and concept_name == node.name:
            if instance_name == node.name:
                return node

            score = get_nx_namefit(instance_name, node.name)
            if score > -1:
                best_match = node

return best_match

With the early-return, we will prefer direct name match over concept match anyway.

rettigl · 2025-03-17T08:29:00Z

@rettigl why was you last commit (25e0013) neccessary? I think we can just do:

best_match = None

for node in nodes:
    if not node.variadic:
        if instance_name == node.name:
            return node
    else:
        if concept_name and concept_name == node.name:
            if instance_name == node.name:
                return node

            score = get_nx_namefit(instance_name, node.name)
            if score > -1:
                best_match = node

return best_match

With the early-return, we will prefer direct name match over concept match anyway.

This might also work. With the code before there were randomly different behaviors, as the elements in the list appear randomly ordered. So we need to parse all emements before we can make a decision. the code above should also work, I guess. The missing documentation for data/energy/@type here: https://github.com/FAIRmat-NFDI/pynxtools/actions/runs/13865392531/job/38803289191#step:9:230 was due to the wrong mapping of AXISNAME[energy] to AXISNAME rather than energy.

lukaspie · 2025-03-17T09:13:55Z

@rettigl why was you last commit (25e0013) neccessary? I think we can just do:
best_match = None

for node in nodes:
    if not node.variadic:
        if instance_name == node.name:
            return node
    else:
        if concept_name and concept_name == node.name:
            if instance_name == node.name:
                return node

            score = get_nx_namefit(instance_name, node.name)
            if score > -1:
                best_match = node

return best_match
With the early-return, we will prefer direct name match over concept match anyway.
This might also work. With the code before there were randomly different behaviors, as the elements in the list appear randomly ordered. So we need to parse all emements before we can make a decision. the code above should also work, I guess. The missing documentation for data/energy/@type here: FAIRmat-NFDI/pynxtools/actions/runs/13865392531/job/38803289191#step:9:230 was due to the wrong mapping of AXISNAME[energy] to AXISNAME rather than energy.

Ok, the new version also seems to work (indeed, my old code was not iterating all nodes before making a decisions, but this is fixed now).

From what I can tell, all the remanining issues with the plugin tests are either solved by using the nameType properly or they are real issues that must be fixed within the plugins. My suggestion is that we merge here (either directly to master or through #585), with the plugin tests failing. Then I can rebase #578 and we can make the plugin updates using the new definitions.

rettigl

Lgtm. Merge here, and close the others

RubelMozumder and others added 17 commits February 18, 2025 15:11

Fix the nx_char type for numpy to and .

0f46c34

Still char instead of the int is being validated which is wrong.

dd8beb4

Remove auto conversion for datatype.

351f377

extends tests.

00b78aa

resolve PR comments

3ad40a0

Remove unnecessary returned value.

6cf6f6d

fix np integer and float.

539bdea

minor change.

cde7a6e

check enums for documented fields, and don't return False if any issu…

4ab4b53

…e with documented keys

add enum checking for attributes

ae10916

Adds parsing code for enumeration tree generation during validation

8f4291e

Fix typos in old test

099b965

always check data types and enums, and check NXdata attributes separa…

4da5735

…tely

fix typos

d4f4c2f

move enum checking into is_valid_data_field, and proper bool conversion

1925d5d

satisfy mypy

ecb4914

mark namefitted group as undocumented

3a1703e

lukaspie requested a review from rettigl March 3, 2025 10:25

lukaspie linked an issue Mar 3, 2025 that may be closed by this pull request

[Bug]: converter does not warn if variadic concept name is invalid #563

Closed

rettigl approved these changes Mar 3, 2025

View reviewed changes

src/pynxtools/dataconverter/validation.py Outdated Show resolved Hide resolved

rettigl and others added 2 commits March 4, 2025 11:45

Merge pull request #565 from FAIRmat-NFDI/fix_check_undocumented

2fae4de

Fix for checking of undocumented keys

add tests from branch fix_attribute_enum_check

4a29251

lukaspie linked an issue Mar 4, 2025 that may be closed by this pull request

Improve search for fields when they are not directly in the appdef #386

Closed

add review suggestion

31cd131

sherjeelshabih added 2 commits March 5, 2025 10:51

Merge pull request #573 from FAIRmat-NFDI/add_more_tests

ca1ef4f

Add more tests

Fixes the types and removes bytes from NX_char as that creates failures

3aabe0b

rettigl added 5 commits March 13, 2025 22:36

revert nomad example test - open enums not working yet

7de7d5a

don't check for units if no good namefit or a group

3f256a4

allow multiple error messages and test for additional error messages

bdf8997

add test for wrong namefitting

eb5f8f6

Merge remote-tracking branch 'origin/fix_validation_and_add_more_test…

802c394

…s' into 563-bug-converter-does-not-warn-if-variadic-concept-name-is-invalid

rettigl changed the base branch from fix_validation_and_add_more_tests to master March 14, 2025 08:54

trigger workflow

25656bd

rettigl and others added 6 commits March 14, 2025 11:32

fix reader tests

8829f06

catch failing namefitting

501537d

tread undocumented units as warning

55397df

require concept_name fornamefitting

6828e03

add test for namefitting with typo in concept_name

a493e1a

remove test comment

3371825

add proper handling of group attributes

d317a7e

rettigl added 2 commits March 14, 2025 22:08

add exception for @url, and run tests on update branch

a018c9b

prefer direct name match over concept match reliably

25e0013

streamline best_namefit_of function

f24e9e0

rettigl approved these changes Mar 17, 2025

View reviewed changes

lukaspie merged commit b83c55e into master Mar 17, 2025
6 of 17 checks passed

lukaspie deleted the 563-bug-converter-does-not-warn-if-variadic-concept-name-is-invalid branch March 17, 2025 09:23

rettigl mentioned this pull request Mar 20, 2025

[Bug]: Dataconverter validation very slow for large arrays #593

Closed

lukaspie mentioned this pull request Apr 7, 2025

[Bug]: Invalid groups with linked children don't get detected as undocumented #429

Closed

rettigl mentioned this pull request May 21, 2025

Enumerations not in appdef are not checked #434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Mark namefitted group as undocumented #570

Mark namefitted group as undocumented #570

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mark namefitted group as undocumented #570

Mark namefitted group as undocumented #570

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!