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

VisItDataCollection broken for L2 fields as of visit 3.4.0 #4280

Closed
sethwatts opened this issue May 2, 2024 · 10 comments
Closed

VisItDataCollection broken for L2 fields as of visit 3.4.0 #4280

sethwatts opened this issue May 2, 2024 · 10 comments
Assignees
Labels

Comments

@sethwatts
Copy link
Contributor

I opened an issue on the Visit team's GitHub due to problems rendering zeroth-order L2 fields saved as an mfem::VisItDataCollection seen in visit 3.4.1 but not noticed in visit 3.3.3. The Visit team concluded the problem was with the saved data file, noting that they had changed expectations for the field association as of visit 3.4.0.

If my understanding of the root cause is correct, MFEM should update the VisItDataCollection::RegisterDataCollection method (circa line 415 of fem/datacollection.cpp) to consider whether the field being registered is an L2 field, and if so, switch the association, which is now hard coded as "nodes" to "elements".

I can contribute a PR making this change, if that is of interest. I'm not sure whether other types of fields require similar changes.

@tzanio
Copy link
Member

tzanio commented May 3, 2024

@cyrush -- is @sethwatts's suggestion to change association of L2 fields from "nodes" to "elements" correct?

I thought in the high-order case everything is considered "nodes"?

@tzanio tzanio added the mesh label May 3, 2024
@cyrush
Copy link
Contributor

cyrush commented May 3, 2024

Yes, this is correct.

In this case "nodes" refers to the association of the field on the mesh via the vertices -- not the high order nodes.

If we want L2 fields to be constant with in an element, we need to change the metadata.

Nodes was an bag/ambiguous choice for this name many years ago.
Default versions of VisIt at LLNL are now using the new mfem-lor, so that is why we noticed this case.

@sethwatts
Copy link
Contributor Author

@cyrush, to clarify, is this association with "elements" only correct for constant L2 fields? If we have, say, a piecewise quadratic DG representation, should that association remain with "nodes"?

@cyrush
Copy link
Contributor

cyrush commented May 3, 2024

I think that is correct.
The info is used to tell VisIt the type of output post LOR: element or vertex associated.

Element associated will always be piecewise constant, where vertex will be interpolated between vertices.

@tzanio
Copy link
Member

tzanio commented May 3, 2024

@cyrush and @sethwatts -- we are about to freeze the mfem-4.7 release, is this something that is critical to add to it? If so, can you propose a fix today?

@sethwatts
Copy link
Contributor Author

I can't propose a fix today. However, I don't think it's essential to get out for mfem-4.7 since using visit-3.3 is a viable (if temporary) solution. I also think topology optimization folks are more likely to be rendering piecewise constant fields than the typical MFEM user who wants to look at solution fields in H1 or similar spaces.

tzanio added a commit that referenced this issue May 3, 2024
@tzanio tzanio mentioned this issue May 3, 2024
@tzanio
Copy link
Member

tzanio commented May 3, 2024

Is this sufficient: #4282

@v-dobrev
Copy link
Member

v-dobrev commented May 3, 2024

I'm not sure why "... they had changed expectations for the field association as of visit 3.4.0" -- the MFEM output has not changed, why change the expectations in the MFEM reader in VisIt?

@cyrush
Copy link
Contributor

cyrush commented May 6, 2024

Now that we are leveraging better MFEM LOR algorithm -- we need proper metadata.

MFEM output metadata isn't correct b/c it is limited to what was implemented many many years ago. (this was effectively only one answer). We designed it to have a way to provide this info, now we just need to update logic in mfem for the pw-constant output cases.

Solution in #4282 looks good to me.

@tzanio
Copy link
Member

tzanio commented May 11, 2024

See the discussion in #4282 and in particular this comment.

@tzanio tzanio closed this as completed May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants