make blueprint helpers available to multiple source files #713

xjrc · 2021-02-19T16:57:28Z

This pull request aims to clean up and abstract the Blueprint helper code (e.g. internal code for querying information about a mesh, like its dimsnion, striding, etc.) so that it's available to source code outside of conduit_blueprint_mesh.cpp. This refactoring will be particularly useful when tackling #682 and finalizing #642 as it will make this helper functionality available to the conduit_blueprint_mpi_mesh.cpp source code.

At a more fine-grained level, this pull request includes the following modifications:

Refactor all Blueprint static containers to use initializer lists.
Create a new internal object file that is imported by both conduit_blueprint_mesh.cpp and conduit_blueprint_mpi_mesh.cpp.
Incorporate a small amount of Blueprint helper code into conduit_blueprint_mpi_mesh.cpp and write associated test cases.

coveralls · 2021-02-19T17:34:40Z

Coverage decreased (-0.2%) to 86.306% when pulling 1aad7da on xjrc:refactor/blueprint-abstract-helpers into 6353066 on LLNL:develop.

xjrc · 2021-03-02T22:43:33Z

@cyrush, @nselliott: I think that this PR is ready for review. Here's an overview of the main changes:

The Blueprint topology utility code that lived in conduit_blueprint_mesh.cpp has been moved to conduit_blueprint_util_mesh.cpp, and exposed via conduit_blueprint_util_mesh.hpp. This header can be used from within any Blueprint source file.
The conduit_blueprint_util_o2mrelation.hpp is an analogous module for general one-to-many utilities (currently just constant variables).
The conduit::blueprint::mesh::coordset::length and conduit::blueprint::mesh::topology::{dims|length} functions have been added to the public Blueprint Mesh interface.
Many test cases have been refactored to use the new constants/functions defined in conduit_blueprint_util_mesh.hpp and conduit_blueprint_util_o2mrelation.hpp.

This should help with #682 and some of the abstractions @nselliott has been working on. Let me know what you think!

nselliott · 2021-03-02T23:26:53Z

src/libs/blueprint/conduit_blueprint_util_o2mrelation.cpp

+/// file: conduit_blueprint_util_mesh.cpp
+///
+//-----------------------------------------------------------------------------
+


Since this file has no code that does anything, are you including it as a place for further additions?

That's correct; I want the small amount of code that's in conduit_blueprint_util_o2mrelation.hpp and I figured that I'd include a corresponding source file just in case we ever need to put anything in it.

nselliott

Thanks @xjrc , this is good.

cyrush · 2021-03-05T23:05:16Z

Joe looks good, One overarching question I have

Should we use:

util::mesh or mesh::utils

I feel like a case could be made for mesh::utils -- they are utils that apply to things that confirm to the mesh bp?

Maybe the same should apply to o2m, but I didn't catch it there?

Want to make sure we consider it.

xjrc · 2021-03-08T16:49:04Z

Joe looks good, One overarching question I have

Should we use:

util::mesh or mesh::utils

I feel like a case could be made for mesh::utils -- they are utils that apply to things that confirm to the mesh bp?

Maybe the same should apply to o2m, but I didn't catch it there?

Want to make sure we consider it.

I like the look of mesh::utils better, and I think it better describes the scope of the functionality (though, some things may ultimately live at some higher blueprint::utils level); I will make this change.

xjrc · 2021-03-08T19:11:04Z

@cyrush: I just remembered why I chose the current ordering: to better match with the nesting scheme used for blueprint::mpi::mesh. I still prefer your naming suggestion, though I suppose my question would be how does the utils namespace differ from the mpi namespace to justify the difference in ordering? If there is no substantial difference, my preference would be to reverse the order in both places (i.e. blueprint::mesh::mpi and blueprint::mesh::utils).

cyrush · 2021-03-08T19:53:57Z

Hmm, lets ponder a bit more.

…o 'conduit_blueprint_util_mesh'.

…nate the unused 'coords' function. * Integrated the 'blueprint::util::mesh' constants into the BP meshing test cases. * Made all of the 'blueprint::util::mesh' constants all caps to match Conduit convention.

… 'blueprint_util_mesh' module.

…non-const 'generate_offsets'. * Added regression tests for the new public Blueprint query functions in 't_blueprint_mesh_query.cpp'.

cyrush · 2021-03-16T00:13:44Z

Still not sure what is best here.

Note: Everything under blueprint::mpi exists in the mpi-enabled lib -- that's one type of logical grouping that is useful in a different way b/c it has linking implications.

Also what do we think about blueprint::mesh::utils blueprint::mpi::mesh::utils?

Honestly -- we can also consider placing those methods simply in blueprint::mesh, unless you are worried they will clutter the namespace? Lots of choices to consider (?)

xjrc · 2021-03-16T17:00:32Z

Still not sure what is best here.

Note: Everything under blueprint::mpi exists in the mpi-enabled lib -- that's one type of logical grouping that is useful in a different way b/c it has linking implications.

That seems to me like a reasonable factor of differentiation.

Also what do we think about blueprint::mesh::utils blueprint::mpi::mesh::utils?

Honestly -- we can also consider placing those methods simply in blueprint::mesh, unless you are worried they will clutter the namespace? Lots of choices to consider (?)

I think that the former is fine, and I certainly prefer it to the latter. I'd like to separate out these functions to help identify them as utilities that have lower-level use cases and interfaces. I will change the namespace ordering.

…o2mrelation}::util'.'

cyrush · 2021-03-16T18:04:38Z

sorry one more nitpick: we have conduit::utils, so should these be utils

…mrelation}::utils'.

xjrc · 2021-03-16T19:01:32Z

sorry one more nitpick: we have conduit::utils, so should these be utils

Point well taken; the changes have been made.

cyrush

thanks @xjrc , looks good!

xjrc · 2021-03-16T22:23:06Z

@cyrush: Can you merge (if Travis looks okay; it failed on grabbing packages, so it's likely on their end)? It looks like I don't have the proper permissions.

cyrush · 2021-03-16T22:25:42Z

yes, I just re-started travis.

But it is strange you can't merge, we should figure that out.

xjrc added task wip labels Feb 19, 2021

xjrc added this to the 0.8.0 milestone Feb 19, 2021

xjrc self-assigned this Feb 19, 2021

xjrc force-pushed the refactor/blueprint-abstract-helpers branch from ceeeb6d to 9f16af9 Compare February 22, 2021 20:21

xjrc removed the wip label Mar 2, 2021

xjrc requested review from cyrush and nselliott March 2, 2021 22:34

nselliott reviewed Mar 2, 2021

View reviewed changes

nselliott approved these changes Mar 3, 2021

View reviewed changes

xjrc closed this Mar 8, 2021

xjrc reopened this Mar 8, 2021

xjrc added 5 commits March 10, 2021 10:58

* Testing out adoption of initializer lists in Blueprint helper code.

2e36ce8

* Abstracted advanced functionality from 'conduit_blueprint_mesh' int…

25c1444

…o 'conduit_blueprint_util_mesh'.

* Refactored the 'blueprint::util::mesh::coordset' functions to elimi…

75fd15e

…nate the unused 'coords' function. * Integrated the 'blueprint::util::mesh' constants into the BP meshing test cases. * Made all of the 'blueprint::util::mesh' constants all caps to match Conduit convention.

* Added 'blueprint_util_o2mrelation' module to serve as equivalent to…

f3a4ce6

… 'blueprint_util_mesh' module.

* Refactored 'offsets' Blueprint utility functions to just const and …

666c0be

…non-const 'generate_offsets'. * Added regression tests for the new public Blueprint query functions in 't_blueprint_mesh_query.cpp'.

xjrc force-pushed the refactor/blueprint-abstract-helpers branch from d9753ea to 666c0be Compare March 10, 2021 18:58

* Changed 'blueprint::util::{mesh|o2mrelation}' to 'blueprint::{mesh|…

544955c

…o2mrelation}::util'.'

* Moved 'blueprint::{mesh|o2mrelation}::util' to 'blueprint::{mesh|o2…

1aad7da

…mrelation}::utils'.

cyrush approved these changes Mar 16, 2021

View reviewed changes

cyrush merged commit 8a0591a into LLNL:develop Mar 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make blueprint helpers available to multiple source files #713

make blueprint helpers available to multiple source files #713

xjrc commented Feb 19, 2021 •

edited

Loading

coveralls commented Feb 19, 2021 •

edited

Loading

xjrc commented Mar 2, 2021 •

edited

Loading

nselliott Mar 2, 2021

xjrc Mar 2, 2021

nselliott left a comment

cyrush commented Mar 5, 2021

xjrc commented Mar 8, 2021 •

edited

Loading

xjrc commented Mar 8, 2021

cyrush commented Mar 8, 2021

cyrush commented Mar 16, 2021

xjrc commented Mar 16, 2021

cyrush commented Mar 16, 2021

xjrc commented Mar 16, 2021

cyrush left a comment

xjrc commented Mar 16, 2021

cyrush commented Mar 16, 2021

make blueprint helpers available to multiple source files #713

make blueprint helpers available to multiple source files #713

Conversation

xjrc commented Feb 19, 2021 • edited Loading

coveralls commented Feb 19, 2021 • edited Loading

xjrc commented Mar 2, 2021 • edited Loading

nselliott Mar 2, 2021

Choose a reason for hiding this comment

xjrc Mar 2, 2021

Choose a reason for hiding this comment

nselliott left a comment

Choose a reason for hiding this comment

cyrush commented Mar 5, 2021

xjrc commented Mar 8, 2021 • edited Loading

xjrc commented Mar 8, 2021

cyrush commented Mar 8, 2021

cyrush commented Mar 16, 2021

xjrc commented Mar 16, 2021

cyrush commented Mar 16, 2021

xjrc commented Mar 16, 2021

cyrush left a comment

Choose a reason for hiding this comment

xjrc commented Mar 16, 2021

cyrush commented Mar 16, 2021

xjrc commented Feb 19, 2021 •

edited

Loading

coveralls commented Feb 19, 2021 •

edited

Loading

xjrc commented Mar 2, 2021 •

edited

Loading

xjrc commented Mar 8, 2021 •

edited

Loading