Initial implementation of support for complex fields #234

wrtobin · 2019-07-01T13:41:13Z

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.

…ds to be made about apf::Field becuase it assumes that the data it is storing is of type double

…freezing

wrtobin · 2019-07-01T13:42:42Z

apf/apfComplex.h

+  #include <complex.h>
+#endif
+
+#define CXX_COMPLEX 1


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

wrtobin · 2019-07-01T13:44:55Z

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

wrtobin · 2019-07-01T13:46:13Z

pumi/pumi_ghost.cc

@@ -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);


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

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

jacobmerson

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

jacobmerson · 2019-07-01T22:38:43Z

apf/apf.h

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

+
+
+


why is there all this extra whitespace?

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

jacobmerson · 2019-07-01T22:42:44Z

apf/apf.h

@@ -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);


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?

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.

jacobmerson · 2019-07-01T22:47:59Z

pumi/pumi_ghost.cc

@@ -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);


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

apf/CMakeLists.txt

cwsmith · 2019-07-03T16:54:01Z

@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 · 2019-07-03T17:48:27Z

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 · 2019-07-03T17:55:48Z

Cameron, Yes we have apf and gmi wrappers for capstone. But those are on a separate private repo. I will take care of those changes if they are needed.

…

On Wed, Jul 3, 2019, 12:54 PM Cameron Smith ***@***.***> wrote: @cwsmith <https://github.com/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 <https://github.com/mortezah> Do we have a capstone implementation of apf_mesh? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#234?email_source=notifications&email_token=AAKDQVAWJD45FTM7YZY2F33P5TKSVA5CNFSM4H4SQSRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFBYOA#issuecomment-508173368>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKDQVE4NQO5HBAVZMZH2MTP5TKSVANCNFSM4H4SQSRA> .

cwsmith · 2019-07-03T18:45:04Z

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 · 2019-07-05T14:21:20Z

@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 · 2019-07-05T14:42:58Z

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?

…field get/set functions

jacobmerson · 2019-07-08T15:26:49Z

apf/apf.cc

@@ -163,13 +161,15 @@ void destroyField(Field* f)

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


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

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.

jacobmerson · 2019-07-08T15:28:45Z

apf/apf.h

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


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

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).

…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 · 2019-07-08T16:53:55Z

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.

apf/apfComplex.h

…citons, changing field zeroing and the element/elementof class hierarchy, adding complex field api to the mesh class, etc

…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 · 2019-07-09T22:56:17Z

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 · 2019-07-10T00:11:24Z

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

wrtobin · 2019-07-10T01:36:27Z

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 · 2019-07-10T13:17:18Z

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

apf/apfElementOf.h

crv/crvQuality.cc

…nstream without needing to carry along a configuration flag

wrtobin · 2019-07-10T14:43:36Z

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

wrtobin · 2019-07-10T15:28:17Z

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 · 2019-07-10T20:08:36Z

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 · 2019-07-11T22:38:08Z

@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 · 2019-07-12T11:52:59Z

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 · 2019-07-12T13:25:15Z

@wrtobin OK. Let's submit a ticket.

William Tobin added 2 commits June 28, 2019 19:41

changing and adding underlying types, at a point where a decision nee…

e72dfe2

…ds to be made about apf::Field becuase it assumes that the data it is storing is of type double

adding complex field files and accomodating complex field freezing/un…

c8ac116

…freezing

wrtobin requested a review from cwsmith July 1, 2019 13:41

wrtobin commented Jul 1, 2019

View reviewed changes

jacobmerson reviewed Jul 1, 2019

View reviewed changes

cwsmith mentioned this pull request Jul 3, 2019

pumi version 2.2.0 -> 2.2.1 #219

Closed

cwsmith requested changes Jul 3, 2019

View reviewed changes

apf/CMakeLists.txt Show resolved Hide resolved

wrtobin changed the base branch from master to develop July 3, 2019 17:45

wrtobin mentioned this pull request Jul 8, 2019

Field get/set functions, type safety, and violating containment #239

Open

William Tobin added 2 commits July 8, 2019 08:08

remove template function from API, add some type-checking to various …

dc9324c

…field get/set functions

closing parenthesis, gotta squash this one for sure

1f09105

jacobmerson reviewed Jul 8, 2019

View reviewed changes

William Tobin added 3 commits July 8, 2019 12:38

enforcing a small amount of type safety to get/set calls in the API, …

246a843

…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)

adding an ElementBase class and an intermediate ElementT class simila…

80c1c68

…r to the FieldBase->(Real)Field/ComplexField hierarchy

implementation of the initial complex fields API, including extension…

07965d0

… of the Mesh API (though no downstream changes should be required)

wrtobin commented Jul 8, 2019

View reviewed changes

apf/apfComplex.h Show resolved Hide resolved

William Tobin added 4 commits July 9, 2019 18:17

quite a bit of work, reverting the type-checks on the API get/set fun…

97a6412

…citons, changing field zeroing and the element/elementof class hierarchy, adding complex field api to the mesh class, etc

reverting accidental change of pField->apf::Field inside of pumi

b587a25

adding apfComplexType.h to public interface

91f67a9

adding new public headers to the pkg_tribits.cmake file

99423cc

adding CMake option to determine compex type, C-complex is the defaul…

63744be

…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

William Tobin added 3 commits July 9, 2019 20:12

forgot a file

0b151f8

removing reference to an unneeded file not in the repo

e8c761b

fixing include issue caused by resolving unneeded intermediate file

5624ba1

addining a public header to the install list as it is needed downstream

3aa1ebe

cwsmith requested changes Jul 10, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

apf/apfElementOf.h Outdated Show resolved Hide resolved

crv/crvQuality.cc Show resolved Hide resolved

removing commented line of code, propogating the complex decision dow…

a9ebda5

…nstream without needing to carry along a configuration flag

wrtobin mentioned this pull request Jul 10, 2019

AggregateNumbering #158

Open

jacobmerson mentioned this pull request Sep 10, 2019

Writing tags to a mesh without having to convert to a field #250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of support for complex fields #234

Initial implementation of support for complex fields #234

wrtobin commented Jul 1, 2019

wrtobin Jul 1, 2019

wrtobin commented Jul 1, 2019

wrtobin Jul 1, 2019

jacobmerson Jul 1, 2019

jacobmerson left a comment

jacobmerson Jul 1, 2019

wrtobin Jul 5, 2019

jacobmerson Jul 1, 2019

wrtobin Jul 3, 2019

jacobmerson Jul 1, 2019

cwsmith commented Jul 3, 2019 •

edited

Loading

wrtobin commented Jul 3, 2019

mortezah commented Jul 3, 2019 via email

cwsmith commented Jul 3, 2019

wrtobin commented Jul 5, 2019 •

edited

Loading

wrtobin commented Jul 5, 2019 •

edited

Loading

jacobmerson Jul 8, 2019

wrtobin Jul 8, 2019

jacobmerson Jul 8, 2019

wrtobin Jul 8, 2019

wrtobin commented Jul 8, 2019 •

edited

Loading

wrtobin commented Jul 9, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

cwsmith commented Jul 11, 2019

wrtobin commented Jul 12, 2019 •

edited

Loading

cwsmith commented Jul 12, 2019

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

Initial implementation of support for complex fields #234

Are you sure you want to change the base?

Initial implementation of support for complex fields #234

Conversation

wrtobin commented Jul 1, 2019

Choose a reason for hiding this comment

wrtobin commented Jul 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobmerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwsmith commented Jul 3, 2019 • edited Loading

wrtobin commented Jul 3, 2019

mortezah commented Jul 3, 2019 via email

cwsmith commented Jul 3, 2019

wrtobin commented Jul 5, 2019 • edited Loading

wrtobin commented Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrtobin commented Jul 8, 2019 • edited Loading

wrtobin commented Jul 9, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

wrtobin commented Jul 10, 2019

cwsmith commented Jul 11, 2019

wrtobin commented Jul 12, 2019 • edited Loading

cwsmith commented Jul 12, 2019

cwsmith commented Jul 3, 2019 •

edited

Loading

wrtobin commented Jul 5, 2019 •

edited

Loading

wrtobin commented Jul 5, 2019 •

edited

Loading

wrtobin commented Jul 8, 2019 •

edited

Loading

wrtobin commented Jul 12, 2019 •

edited

Loading