-
Notifications
You must be signed in to change notification settings - Fork 56
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
Revisiting non-FAIRmat constructive solid geometry in light of FAIRmat proposal #1532
Open
mkuehbach
wants to merge
13
commits into
main
Choose a base branch
from
issue1531_nxcsg
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
…geometry can be used to define geometries as instances
… that class to a base class
This was referenced Jan 13, 2025
Open
Closed
Closed
PeterC-DLS
reviewed
Jan 14, 2025
base_classes/NXcsg.nxdl.xml
Outdated
@@ -30,13 +30,13 @@ | |||
xsi:schemaLocation="http://definition.nexusformat.org/nxdl/3.1 ../nxdl.xsd" | |||
> | |||
<doc> | |||
Constructive Solid Geometry base class, using :ref:`NXquadric` and :ref:`NXoff_geometry`</doc> | |||
Constructive Solid Geometry base class</doc> |
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.
State it combines the definition of leaf and branching nodes of a csg tree.
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.
Implemented
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issues #1529, #1530, and #1531 discuss that NeXus provides classes for constructive solid geometry.
Despite their existence they have not been used much. Therefore, it was proposed to deprecate them.
However, their design is solid and having the possibility for defining CSG is useful. Therefore, I suggest to
keep them in this slightly edited style.
Going further, I propose here also to promote them to base classes as they have been sitting in contributed
too long but there is no other substantiated argument as to why these are so poor to stay in contributed
forever.
In discussion with @PeterC-DLS we thought that the FAIRmat proposal via #1421 introduces a wider set of
base classes that has been designed with offering a potentially easier entry point for people not everyday
dealing with computational geometry. As #1421 extends NXoff_geometry by dedicated base classes with
more fields it makes sense to allow that some of these base classes, like NXcg_polyhedron, NXcg_tetrahedron,
could be leaf nodes of CSG constructions, therefore I edited the docstrings for the elements of the CSG tree.
I propose to merge these changes via one PR and this issue. Consequently, I propose to close #1529 and #1530
via comment pointing to #1531 here.