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

Initial implementation of support for complex fields #234

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

wrtobin
Copy link
Collaborator

@wrtobin wrtobin commented Jul 1, 2019

Need to:

  • add a test case to CTest
  • squash the commits once the test is passing

Just wanted to get eyes on this to see if anyone can spot something I'm breaking or should do differently that isn't obvious to me.

William Tobin added 2 commits June 28, 2019 19:41
…ds to be made about apf::Field becuase it assumes that the data it is storing is of type double
@wrtobin wrtobin requested a review from cwsmith July 1, 2019 13:41
apf/apfComplex.h Outdated
#include <complex.h>
#endif

#define CXX_COMPLEX 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah, need to add this as a CMake option rather that just hard defining it to be CXX_COMPLEX for now.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 1, 2019

Oh yeah I need to add a couple functions to the apf Simmetrix mesh class.

@cwsmith aside from mds and simmetrix do we wrap any other mesh types that I'll need to modify either in CORE or downstream in associated projects?

I could swap the functions from being pure-virtual to just virtual with a default that fail("Unimplemented")-s

@@ -400,7 +400,7 @@ void pumi_ghost_create(pMesh m, Ghosting* plan)
std::vector<apf::Field*> frozen_fields;
for (int i=0; i<m->countFields(); ++i)
{
pField f = m->getField(i);
apf::Field * f = m->getField(i);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should revert this, it crept in while I was working on an issue with freezing fields with FieldDataOf where T != double.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should keep the new version since it is consistent with the rest of the (non pumi) codebase

Copy link
Contributor

@jacobmerson jacobmerson left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable approach that follows what was done for the other field types.

apf/apf.h Outdated
@@ -704,6 +706,9 @@ bool isPrintable(Field* f);
*/
void fail(const char* why) __attribute__((noreturn));




Copy link
Contributor

Choose a reason for hiding this comment

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

why is there all this extra whitespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just crept into the commit, I've fixed this and once I squash the commits it will not be in the merge

apf/apf.h Outdated
@@ -713,12 +718,15 @@ void unfreeze(Field* f);
/** \brief Returns true iff the Field uses array storage. */
bool isFrozen(Field* f);

template <typename T>
T* getArrayDataT(Field * f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go into the cc file and use template specializations, as to not confuse the user space interface? If not possibly add a comment mentioning that this shouldn't be directly called by user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that's probably the way to go. Don't necessarily want users exposed to this template function. Wrapping up use of the template function in a couple of interface functions similar to how getArrayData is now implemented is probably the best short-term solution.

Unfortunately there is a potential for misuse if a user supplies a 'Field' to one of these functions with the wrong underlying type, since we basically have to rely on a reinterpret_cast to get the underlying data of the different type. I'll see if I can add an assert or something to try to prevent this issue.

@@ -400,7 +400,7 @@ void pumi_ghost_create(pMesh m, Ghosting* plan)
std::vector<apf::Field*> frozen_fields;
for (int i=0; i<m->countFields(); ++i)
{
pField f = m->getField(i);
apf::Field * f = m->getField(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should keep the new version since it is consistent with the rest of the (non pumi) codebase

apf/CMakeLists.txt Show resolved Hide resolved
@cwsmith
Copy link
Contributor

cwsmith commented Jul 3, 2019

@cwsmith aside from mds and simmetrix do we wrap any other mesh types that I'll need to modify either in CORE or downstream in associated projects?

@mortezah Do we have a capstone implementation of the apf mesh interface?

@wrtobin wrtobin changed the base branch from master to develop July 3, 2019 17:45
@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 3, 2019

Switched base branch to develop cause I'm a dummy. Is there a way to restrict users from creating PRs for a branch (master in this case)?

@mortezah
Copy link
Contributor

mortezah commented Jul 3, 2019 via email

@cwsmith
Copy link
Contributor

cwsmith commented Jul 3, 2019

Is there a way to restrict users from creating PRs for a branch (master in this case)?

@wrtobin The only option I'm seeing in the github interface is to change the 'default' branch:

"The default branch is considered the “base” branch in your repository, against which all pull requests and code commits are automatically made, unless you specify a different branch."

I don't like this approach though as it will result in users who do a fresh clone getting placed into the develop branch instead of the stable master branch.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 5, 2019

@cwsmith Well at least only a repo admin can screw it up (looking at me, @wrtobin)

Anyway I'm working on cleaning up the implementation and implementing a test case, but have a conceptual issue.

While FieldDataOf<T> allows us to store fields with arbitrary data types (though is currently restricted to the Mesh::TagType enum types), the primary API is very much built around the Field* type which is assumed to contain a FieldDataOf<double>, though the actually-stored types might be any from the apf::ValueType enum which has SCALAR, VECTOR, MATRIX, PACKED (in the develop branch), which aren't scalar types, they are intermediate data structures which all hold underlying double values, the actual scalar type of the field.

To implement the complex field I added a COMPLEX_PACKED value to that enum, which is passed through the typical field making APIs and results in the Field* having a FieldDataOf<double_complex> internally.

The issue is somewhat twofold: (1) this new field violates the assumption that all Field objects store type double, (even though the underlying class hierarchy allows for this, the API is build around this assumption in many ways), and (2) adding COMPLEX_PACKED to the ValueType enum is changing the fundamental purpose of the enum from expressing the data structure used by the field to expressing the scalar type of the field.

I'm trying to reconcile these issues and come up with the cleanest solution. Unfortunately since the API is built around the Field->double assumption, modifying it to allow other scalar types while maintaining the API might require some extensive changes 'under the hood' to do type checking (using a combination of Field::getScalarType(), templating, and std::is_same<>(), along with reinterpret_casting, all of which are not great implementation practices.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 5, 2019

Without a massive amount of work, the functionality we can get from a complex field would probably be:

  • create a complex field
  • freeze a complex field and retrieve the underlying array
  • access complex field data based on mesh adjacency information
  • unfreeze the field

Which is largely enough for the motivating use case in M3D-C1.
It would also be nice to be able to build a complex Element

Assuming this all happens with the default Field* datatype as a 'wrapper' for the complex field, there is potential for misuse by users down the road if they build this field and try to use it in the general API. Many operations can be guarded with asserts or fails, but going though the entire API and checking that every time a Field* is passed around, the getScalarType() == Mesh::DOUBLE before using it is not desirable.

Do we have any mechanism to selectively publish/install headers only in specific 'developer' installations? Is having such a mechanism even a good idea or is it another giant can of worms? @cwsmith any thoughts?

apf/apf.cc Outdated
@@ -163,13 +161,15 @@ void destroyField(Field* f)

void setScalar(Field* f, MeshEntity* e, int node, double value)
{
PCU_DEBUG_ASSERT(f->getValueType() == SCALAR);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are now allowing c++11 in CORE can this be done with a static assert?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Core has been C++11 for a while IIRC. I've been using auto and range-based for's for a while no one has complained so far.

This assert has to be runtime though.

apf/apf.h Outdated
\note If the underlying field data type is NOT double,
this will cause an assert fail in all compile modes.
*/
double* getArrayData(Field* f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a mechanism for depreciation? If so, this should probably be depreciated and replaced with getDoubleArrayData to have a consistent API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since C++14 we have [[deprecated]], though this codebase is explicitly only up to C++11 IIRC and we haven't put in a deprecation mechanism as far as I know.

C++14 was basically just a small step from 11 so moving core to that should be possible, but is a different issue.

For the time being I will introduce a more consistent API function and not in the doxygen this is deprecated (see apfNumbering.h near the bottom for the only other place this has been done I think).

William Tobin added 3 commits July 8, 2019 12:38
…implementing ability to get Element nodes for a packed field, restricting ElementOf<T> to be standard layout so the underlying data retrieval cast is valid (see issue #239)
…r to the FieldBase->(Real)Field/ComplexField hierarchy
… of the Mesh API (though no downstream changes should be required)
@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 8, 2019

Unfortunately the most effective way to get the desired functionality in without too much work cost is to basically replicate core portions of the field API for specifically complex fields since the Field type is explicitly supposed to contain only real-valued fields.

Under the hood where all the templating is, things were easier to implement, moving some things around, creating a few superclasses, and generalizing a few pieces of functionality, before unifying how the API uses the new structures to ensure the only real code duplication going on is the surface level API, since it is 100% the same except for some data types. In fact I kept the function names the same so porting existing algorithms can be a bit easier, just have to change the Field to ComplexField and any double arrays to double_complex.

Still need to write a robust test case and fix some bugs, I'm sure, along with adding the double_complex/std::complex cmake option.

…t, had to change all occurences of the symbol 'I' in the codebase because the c-complex header defines a macro with that name, so we cannot use that symbol anywhere in the codebase
@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 9, 2019

Okay, everything should be done with this PR. All existing test cases are passing, including the new complex test case. We have a cmake option to determine which type of complex to use, and all tests pass in both cases.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

I'm looking into the travis CI issue and hope to resolve it soon.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

Okay now that the automated tests are passing I'm going to squash the commits to clean this up before we consider merging things in.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

Ah okay the squash will happen during the merge of this PR since we have force-pushing disabled on this repo (a smart move).

@cwsmith When you get a chance can you approve the PR/clear your review so I can merge?

CMakeLists.txt Outdated Show resolved Hide resolved
apf/apfElementOf.h Outdated Show resolved Hide resolved
crv/crvQuality.cc Show resolved Hide resolved
…nstream without needing to carry along a configuration flag
@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

Oh I still need to modify apfSIMMesh, let me see about that.

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

So the additions to the simmetrix mesh type are fairly straightforward, except for the migration of complex tags, since simmetrix doesn't provide a general callback function for send/recv of tags in migration, only for integer tags and double tags so far as I can tell. Creating a new callback function would be feasible... if I knew enough about what the functions actually have to accomplish...

@wrtobin wrtobin mentioned this pull request Jul 10, 2019
@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 10, 2019

Do we have anyone who knows about the simmetrix data migration callback functions? Or would it be best to submit a ticket?

Theoretically a user with a simmetrix mesh can still create complex fields, they just can't move meshes entities around at all...

@cwsmith
Copy link
Contributor

cwsmith commented Jul 11, 2019

@wrtobin One of Onkar's students may have used it. This example may be helpful:
https://www.scorec.rpi.edu/~cwsmith/SCOREC/SimModSuite_latest/PartitionedMesh/group__PMEx__Attach.html

@wrtobin
Copy link
Collaborator Author

wrtobin commented Jul 12, 2019

Yeah the issue is these functions:

  MD_setMeshCallback(id, CBmigrateOut, pm_sendDblArray, adc);
  MD_setMeshCallback(id, CBmigrateIn, pm_recvDblArray, adc);

We can get the signature of the pm_[send/recv]DblArray callback functions since that is in the documentation (and the same as the delete callback), but I don't precisely know what those callbacks are supposed to accomplish -- the name certainly gives something away, but I have no knowledge of the semantics involved in implementing these send/recv operations.

Simmetrix implements pm_[send/recv][Dbl/Int]Array, but not a complex version (for either version of complex), nor do they appear to implement a generic pm_[send/recv]SizeOfArray or something similar.

Cursory searches through the relevant documentation don't come up with anything either. Unless we have someone who has written their own version of these callback functions I'm likely going to submit a low-level ticket with simmetrix before the end of the day.

@cwsmith
Copy link
Contributor

cwsmith commented Jul 12, 2019

@wrtobin OK. Let's submit a ticket.

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.

4 participants