Skip to content

feat: Export coordinates node#2417

Open
rprospero wants to merge 17 commits intodevelop2from
export_coordinates_node
Open

feat: Export coordinates node#2417
rprospero wants to merge 17 commits intodevelop2from
export_coordinates_node

Conversation

@rprospero
Copy link
Copy Markdown
Contributor

This adds an ExportCoordinatesNode to go along side the ExportTrajectoryNode.

@rprospero rprospero requested review from RobBuchananCompPhys and trisyoungs and removed request for trisyoungs April 24, 2026 16:22
Copy link
Copy Markdown
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, std::format_to is a nice thing! This all looks fine, but I think we should go a step further and have two nodes, each focussing on a specific format (I also think we should refer to "Configurations" rather than "Coordinates" as we have historically done) - ExportXYZConfiguration and ExportDLPolyConfiguration. This means we separate out options for specific file formats (not that we have any here, but the future is looming....) and remove the CoordinateExportFormat completely.

Comment thread src/nodes/exportCoordinates.h Outdated
Comment thread src/nodes/registry.cpp Outdated
Comment thread src/nodes/exportXYZConfiguration.h
@rprospero rprospero force-pushed the export_coordinates_node branch from 2af87c9 to 56e99cc Compare April 27, 2026 12:29
@rprospero rprospero force-pushed the export_coordinates_node branch from 56e99cc to 4dfc439 Compare April 27, 2026 16:14
@rprospero rprospero requested a review from trisyoungs April 28, 2026 08:34
Copy link
Copy Markdown
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of nitpicky suggestions, but all in good spirit! 😃

Comment on lines +17 to +18
addOption<bool>("TagWithIteration", "Whether to tag (suffix) the filename with the current iteration index",
tagWithIteration_);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we shouldn't mention, or use, "iteration" at all in future? This is not something for consideration in this PR, but this option could morph into something like an "Overwrite" option, which if true just keeps writing to the same (non-suffixed) file, but if false seeks out the next suffix number to use based on the pre-existing files.

Comment thread src/nodes/exportDLPolyConfiguration.cpp Outdated
Comment thread src/nodes/exportDLPolyConfiguration.cpp Outdated
Comment thread src/nodes/exportDLPolyConfiguration.cpp Outdated
Comment thread src/nodes/exportXYZConfiguration.cpp Outdated
Comment thread src/nodes/exportXYZConfiguration.cpp Outdated
Comment thread src/nodes/exportXYZConfiguration.h Outdated
Comment thread src/nodes/exportDLPolyConfiguration.h Outdated
Comment thread src/nodes/exportXYZConfiguration.h Outdated
Comment thread src/nodes/registry.cpp
@trisyoungs
Copy link
Copy Markdown
Member

@rprospero Just noticed that the source files also need renaming ("Coordinates" -> "Configuration") too.

rprospero and others added 12 commits May 5, 2026 09:51
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants