SCOREC
Initial implementation of support for complex fields
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.
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
@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?
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)?
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 [email protected] 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 https://github.com/SCOREC/core/pull/234?email_source=notifications&email_token=AAKDQVAWJD45FTM7YZY2F33P5TKSVA5CNFSM4H4SQSRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZFBYOA#issuecomment-508173368, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKDQVE4NQO5HBAVZMZH2MTP5TKSVANCNFSM4H4SQSRA .
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.
@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.
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?
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
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.
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.
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?
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...
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...
@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
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.