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

implement FieldData::clone() #163

Merged
merged 4 commits into from
Aug 13, 2018
Merged

Conversation

jacobmerson
Copy link
Contributor

this adds an implementation to clone field data which is needed for
apf::convert to copy fields. closes #162

FieldData* FieldData::clone()
{
abort();
FieldData* newData = new TagDataOf<double>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to commit to all copied fields being created as mesh tags?

Certainly all fields using the primary apf interfaces in apf.h are mesh tag fields but I'm not sure we want to have references to a subclass in a superclass. Should clone() be virtual and the type created depend on the subclass type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really up to you and @cwsmith. it was not pure virtual originally, so I just implemented it as that implied.

for (int i=0; i<num_nodes*num_components; i++) {
data[i] = this->dataArray[start+i];
}
template <class T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this all whitespace changes?

If so revert these edits.

FieldData* FieldData::clone()
{
abort();
FieldData* newData = new TagDataOf<double>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be templated, so we don't have to use only a double field.

template <class T>
void unfreezeFieldData(FieldBase* base);
template <class T>
class ArrayDataOf : public FieldDataOf<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this class need to be exposed? Is it used somewhere else in the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake. I was using this in a previous attempt to get this working.

@jacobmerson
Copy link
Contributor Author

@TobinW can you take another look at this. Some of my commits are a bit messy, so if under diff settings you change it to all commits rather than the last one you can see in sum what changed. If things look good to you I will squash all of my commits down into one.

@@ -1,6 +1,9 @@
#include <PCU.h>
#include "apfFieldData.h"
#include "apfShape.h"
#include "apfArrayData.h"
#include "apf.h"
#include "apfTagData.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still need to remove these.

@wrtobin
Copy link
Collaborator

wrtobin commented Aug 1, 2018

@Timetravellers This looks like a better approach to me, just need to remove a couple #include directives.

@cwsmith Anything jumping out at you? FieldData.h is non-public so changing the function to pure-virtual shouldn't break any downstream code and the only interface changes seems to be the additional function for UserData in apf.h

this adds an implementation to clone field data which is needed for
apf::convert to copy fields. closes SCOREC#162
@jacobmerson
Copy link
Contributor Author

@TobinW and @cwsmith updated and squashed. Let me know if there is anything else needed before merge.

@jacobmerson
Copy link
Contributor Author

@TobinW I am finally done with this. We at least clone all mesh data that is used in tobinw/biotissue

@jacobmerson
Copy link
Contributor Author

The one possibility where this breaks downstream code is if the user copies fields/tags etc. on their own. The downstream user no longer needs to copy fields when using apf::makeMdsMesh() or apf::convert().

@jacobmerson
Copy link
Contributor Author

@cwsmith can you merge this? or are there more edits that need to be made?

@cwsmith
Copy link
Contributor

cwsmith commented Aug 8, 2018

I hit a compile error on pachisi using gcc 7.3.0

/lore/cwsmith/develop/core/apf/apfConvert.cc: In member function ‘void apf::Converter::convertTag(apf::Mesh*, apf::MeshTag*, apf::Mesh*, apf::MeshTag*)’:
/lore/cwsmith/develop/core/apf/apfConvert.cc:249:22: error: ‘cerr’ is not a member of ‘std’
                 std::cerr << "Tried to convert unknown tag type\n";
                      ^~~~
/lore/cwsmith/develop/core/apf/apfConvert.cc:249:22: note: suggested alternative: ‘errc’
                 std::cerr << "Tried to convert unknown tag type\n";
                      ^~~~
                      errc
/lore/cwsmith/develop/core/apf/apfConvert.cc: In member function ‘void apf::Converter::convertTags()’:
/lore/cwsmith/develop/core/apf/apfConvert.cc:331:20: error: ‘cerr’ is not a member of ‘std’
               std::cerr << "Tried to convert unknown tag type\n";
                    ^~~~
/lore/cwsmith/develop/core/apf/apfConvert.cc:331:20: note: suggested alternative: ‘errc’
               std::cerr << "Tried to convert unknown tag type\n";
                    ^~~~
                    errc
make[2]: *** [apf/CMakeFiles/apf.dir/apfConvert.cc.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [apf/CMakeFiles/apf.dir/all] Error 2
make: *** [all] Error 2

@jacobmerson
Copy link
Contributor Author

@cwsmith see if that fixes the issue.

@cwsmith cwsmith merged commit 6f3c392 into SCOREC:develop Aug 13, 2018
@cwsmith cwsmith mentioned this pull request Aug 13, 2018
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.

None yet

3 participants