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

Fix for #4280 #4282

Closed
wants to merge 5 commits into from
Closed

Fix for #4280 #4282

wants to merge 5 commits into from

Conversation

tzanio
Copy link
Member

@tzanio tzanio commented May 3, 2024

Resolve #4280

@sethwatts
Copy link
Contributor

This LGTM, @tzanio.

@v-dobrev
Copy link
Member

v-dobrev commented May 4, 2024

I don't like the idea of changing the output format to accommodate a change in the reader, especially if the change is not backward compatible.

I think the better solution is to fix this in the reader. I can do that among other fixes in the reader I'm planning to do (@cyrush is aware of my plans) -- I just have not had time to work on them recently. @cyrush, if I start this work soon, what branch should I use as the starting point? I plan to propose my changes through a fork -- as we've discussed it in the past.

@cyrush
Copy link
Contributor

cyrush commented May 6, 2024

@v-dobrev please see my comment in #4280, the output metadata needs to reflect the type of field the user wants to see. The mfem root file needs to state if the output is vertex associated (nodes -- this is a bad name, but it's the name we have used) or element associated (elements). It does not contain any other metadata about the true basis function, so we need this.

@tzanio tzanio changed the base branch from master to mfem-4.7-dev May 7, 2024 13:36
@tzanio
Copy link
Member Author

tzanio commented May 7, 2024

@sethwatts, we changed this PR after discussion with @cyrush.

Any chance you can try it in your application? We expect that it should resolve your issue and still work in older versions of VisIt, but we would like to confirm before including it in mfem-4.7 (to be released today)

@sethwatts
Copy link
Contributor

@tzanio, I won't be able to rebuild my application with the mfem 4.7 branch today since I have other dependencies which themselves depend on mfem, and I won't have time today to get everything updated correctly. However, I can confirm that changing the assoc field from "nodes" to "elements" in the *.mfem_root files, with no other changes, does indeed fix the visualization issue in visit 3.4.1. Hopefully that is sufficient to include this change to the 4.7 release.

@tzanio
Copy link
Member Author

tzanio commented May 7, 2024

Thanks @sethwatts that's exactly what we need.

One follow-up: does "elements" works also in VisIt 3.3?

@sethwatts
Copy link
Contributor

No, "elements" does not work when rendered with visit-3.3; instead, it throws an error message and does not render anything. As I understood the discussion on visit issue 19469, the change in expectation for "elements" is new in visit-3.4.

@cyrush
Copy link
Contributor

cyrush commented May 7, 2024 via email

Base automatically changed from mfem-4.7-dev to master May 7, 2024 22:56
@tzanio
Copy link
Member Author

tzanio commented May 8, 2024

Since there is no backward-compatible solution, and we don't want to break the support for VisIt 3.3, we will address this by updates in the VisIt plugin.

@sethwatts, we recommend that you either use VisIt 3.3 for now, or add a post-processing step to change "nodes" to "elements" in your root files. I'm happy to explain the rationale for this in more detail if you are interested.

@tzanio tzanio closed this May 8, 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 this pull request may close these issues.

VisItDataCollection broken for L2 fields as of visit 3.4.0
4 participants