Skip to content
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

Check IVTemp example to make sure it correctly complains on missing inherited User group #128

Open
sanbrock opened this issue Apr 5, 2023 · 9 comments
Assignees

Comments

@sanbrock
Copy link
Contributor

sanbrock commented Apr 5, 2023

This is very similar to #112 and should be solved with it or afterwards.

@sherjeelshabih
Copy link
Contributor

sherjeelshabih commented Apr 11, 2023

The converter does not have NXsensor_scan required fields in the generate-template. So it doesn't check for User group, if its not provided. But if a User group is provided, for an instance of NXiv_temp, it gets correctly identified and sorted.

The calibration_time field is not found for example:
"/ENTRY[entry]/INSTRUMENT[instrument]/ENVIRONMENT[environment]/voltage_controller/calibration_time"

  • Check example on Nomad dev

@sherjeelshabih
Copy link
Contributor

It's possible to check whether an AppDef extends and incorporate the parent AppDef as well. The only question is how to handle mods. This is currently not clear how we could handle it, implementation-wise.

@sanbrock
Copy link
Contributor Author

get_inherited_nodes on "/ENTRY/INSTRUMENT/ENVIRONMENT"
works fine and finds inheritance from sensor_scan, too.

Problem is with the next step when looking for "/ENTRY/INSTRUMENT/ENVIRONMENT/voltage_controller" as it is not found in sensor_scan (because only 'SENSOR' is defined there whet we wanted to specialise in iv_temp by requiring the presence of 'voltage_controller').

To allow a new concept (intentionally having an IS_A or SAME_AS relationship to a superclass) be defined under a specialised name (as above), the inheritance check must include checking its type (base class name /NX_CLASS/ in case of a Group or data type in case of a Field) because it is needed to resolve ambiguity according to the NeXus naming convention. (e.g. two groups defined next to one another, but without specifying their names must come from different base classes)

  • "/ENTRY/INSTRUMENT/ENVIRONMENT/SENSOR[voltage_controller]", or
  • "/ENTRY/INSTRUMENT/ENVIRONMENT/voltage_controller" and [NXentry, NXinstrument, NXenvironment, NXsensor]

Note that this information is present in the application definitions, so it should not be a problem to pass it over.

@sanbrock
Copy link
Contributor Author

Actually, I have implemented it with no need to pass over the type, as the code now finds it automatically.

@sherjeelshabih
Copy link
Contributor

Thank you very much! This should help with the merge and get all children function.

I also want to point out that there is one more inheritance issue with NXpid under NXenvironment. All children of the following cannot be found:

  • "/ENTRY[entry]/INSTRUMENT[instrument]/ENVIRONMENT[environment]/NXpid[heating_pid]"

For example:

  • "/ENTRY[entry]/INSTRUMENT[instrument]/ENVIRONMENT[environment]/NXpid[heating_pid]/description"
  • "/ENTRY[entry]/INSTRUMENT[instrument]/ENVIRONMENT[environment]/NXpid[heating_pid]/pv_sensor/value_log/value"

@sherjeelshabih
Copy link
Contributor

sherjeelshabih commented Apr 13, 2023

The commit here actually generates the new template. But this now fails the previously setup pytests. If you like I can update the tests. But first if someone could have another look at the generated templates, it will be nice to see what we have.

The template generation fails for further groups found in parent classes that haven't been mentioned in the AppDef. This happens when trying to access NXgeometry somehow. I didn't have a deeper look. It's better to check if we do need more in this template generation.

@sanbrock
Copy link
Contributor Author

"/ENTRY[entry]/INSTRUMENT[instrument]/ENVIRONMENT[environment]/NXpid[heating_pid]"
should read
"/ENTRY[entry]/INSTRUMENT[instrument]/ENVIRONMENT[environment]/PID[heating_pid]"

@sherjeelshabih
Copy link
Contributor

This is found correctly. Thanks.

@sherjeelshabih
Copy link
Contributor

The relevant MR is here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants