This repository has been archived by the owner on Nov 7, 2024. It is now read-only.
Ecdc 3331 nxoff geometry #1040
Open
Ecdc 3331 nxoff geometry #1040
+112
−78
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danesss
reviewed
Sep 27, 2023
Comment on lines
-87
to
-93
| elif shape_type == GEOMETRY_NX_CLASS: | ||
| for child in self.shape_info[CommonKeys.CHILDREN][0][CommonKeys.CHILDREN]: | ||
| if ( | ||
| child[NodeType.CONFIG][CommonKeys.NAME] == SHAPE_GROUP_NAME | ||
| and child[NodeType.CONFIG][CommonKeys.VALUES] == NX_BOX | ||
| ): | ||
| self._add_box_shape_to_component() |
There was a problem hiding this comment.
I understand that this will break the loading of most existing instrument templates, because they tend to use NXgeometry in some places.
Do we need to drop support for NXgeometry? Or should we just move away from it in newly created files but still load old files correctly?
Comment on lines
+723
to
+724
| assert len(shape.faces[0]) == 4 | ||
| assert len(shape.faces) == 6 |
There was a problem hiding this comment.
Shall we assert the dimensions of the shape like length/width/height?
Comment on lines
+194
to
195
| self.nx_class = OFF_GEOMETRY_NX_CLASS | ||
| self._create_datasets_and_add_to_shape_group() |
There was a problem hiding this comment.
The method _create_datasets_and_add_to_shape_group(self) makes use of SHAPE_NX_CLASS. Is that still adequate when using NXoff_geometry?
If not, then this file may need a bigger change to translate all NXshape geometries to use NXoff_geometry
|
on hold for now |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
https://jira.esss.lu.se/browse/ECDC-3331
We do not want to process geometries other than OFF geometries now. A release has been made as a final version supporting other types. This is in line with the NeXus standard and notes on OFF geometries can be found at https://confluence.esss.lu.se/display/ECDC/NeXus+Constructor
The Constructor almost exclusively uses OFF geometries anyway, but this makes it explicit that it will not be further supported.
Description of work
Three lines have been added to ensure that the code returns gracefully if JSON children are not present in shape processing. Otherwise, code has mostly been removed. In this removal, a tidy-up has been performed to make better use of Python typing methods.
Acceptance Criteria
Fully compatible with JSON that uses OFF geometries with no failures in the geometry section.
UI tests
No tests required except to visually determine the correct processing of a box geometry (which has been included as a unit test)
Nominate for Group Code Review