Fix usage of change_tagToMesh/change_tagTorc

[jacobmerson]

There is a pattern in the vtk writer and unmap where the write_array function is surrounded by calls to change_tagToMesh and change_tagTorc.

There is a change in the set_tags function which makes it invalidate a pointer to the old tag when the array gets swapped. So, using change_tagToMesh/change_tagTorc has become silently dangerous because of the tag invalidation.

I'm not exactly sure what the difference in the formats is, but it seems as though the baseline storage of the reverse classification on the mesh should be in the "rc" format and it's just converted to the mesh format to write it out or do whatever unmap does.

@joshia5 I'd like to get your opinion on the following change because it seems that it will save some work/memory.

1. always store the _rc tags in "rc" format (looks like this is currently the intent)
2. deprecate/remove change_tagToMesh and change_tagTorc
3. provide a new function (please help with the naming) rc_tag_to_mesh_array which returns the rc tag array in the "mesh" format.

[jacobmerson]

Looking at the unmap code I'm wondering why we don't directly store the _rc tags in rc format rather than 1) convert to mesh format, 2) set tag, 3) convert to rc format.

I'm guessing it's due to a check on adding a tag that requires that adding a tag be of a certain size that corresponds to the number of nodes. In that case, why don't we just store the rc tags in a separate cache (not as tags) and have two loops in write_vtk and unmap_mesh one for the normal tags and one for the _rc tags.

I suspect/hope that the user/application code isn't relying on the _rc tag fields and is using the high level mesh.get_RevClass functions.

I will finish getting things functional as is and then submit a pull request to review with these refactors.

[joshia5]

Hi @jacobmerson , we do directly store the _rc tags in rc format. The change_tagToMesh and change_tagTorc (could also be private) functions are there to support maintaining the rc tags through mesh modification operations like adaptation, migration. While writing vtk, one needs to add data fields for all the mesh entities even if the information of interest is associated with a single mesh entity, so we change the tags to mesh before writing to vtk. For rc operations associated with file i/o, i see your point that we dont necessarily need to change the arrays that are stored with the tagbase from rc to mesh and just unmapping the array should suffice but I wrote it the way it is to initially try and remain consistent with the other operations requiring tag data sized the size of the mesh. Does that answer your question?

[jacobmerson]

I think I understand...essentially, the field transfer (and writing procedures) require data in mesh format and storage format is the rc format. You store in tags array to make use of the mesh field transfer operations.

I'm hitting a bug where the size of the arrays are not getting reset. I'm going to try a different approach and see if that can get over the hump.

[joshia5]

okay, sounds good. This is not required in any way but if you have the time, it might be helpful to log the output of rc-test failure with this issue

[jacobmerson]

The error after merging was a failure here. The data looked correct but the array sizes were getting messed up after the write to file here.

I suspected that it's somehow an issue with the tag swapping for a while and then I thought it was just an unmatched conversion, but doesn't seem to be.

You can find the merge with the broken unit tests and some debug outputs here.

As I mentioned earlier. I'm refactoring a bit such that we store the tags in rc format but not in the tags vector of the mesh. Then everywhere possible we work with the rc arrays rather than using tags. I haven't dug into it yet, but as you say we may need the mesh tags for adaption. If so, there is still the same Torc function, but now it works slightly different under the hood.

[jacobmerson]

@joshia5 is there a reason that the rc tag is stored as a Real? Whenever I have dealt with classification in other codes we always make sure to use integers to avoid any floating point issues with the classifications. (granted the round trip int->double->int isn't likely to cause problems).

[joshia5]

@joshia5 is there a reason that the rc tag is stored as a Real? Whenever I have dealt with classification in other codes we always make sure to use integers to avoid any floating point issues with the classifications. (granted the round trip int->double->int isn't likely to cause problems).

I don't follow. rc tags are not stored as Real. Would you be able to add permalink where you find it to be so?

[joshia5]

rc tags are stored as per the user specified type

[jacobmerson]

Fixed in 7dbcd10