search

visit-dav / visit

VisIt - Visualization and Data Analysis for Mesh-based Scientific Data
https://visit.llnl.gov
BSD 3-Clause "New" or "Revised" License
444 stars 117 forks source link

Mili Material Edge Lines #19919

Closed JustinPrivitera closed 1 month ago

JustinPrivitera commented 1 month ago

Description

Resolves #17950 Resolves #19025

We need VisIt to calculate ghost zones using global node ids. VisIt won't calculate ghost zones if there are already ghosts present. The mili plugin creates ghost zones and ghost nodes, so we determined it made sense to plumb the ghost zones and nodes created by the plugin differently. We now pass them through with different names, "avtExtraGhostZones" and "avtExtra GhostNodes". These arrays get stitched together with the newly calculated ghost arrays later in the process.

Type of change

How Has This Been Tested?

built and ran on rzhound toss4. All tests pass in serial and parallel

Reminders:

Checklist:

JustinPrivitera commented 1 month ago

Is a single bool that lives in avtMiliFileFormat.h sufficient to catch node ids not being read? Are there strange parallel cases that could mess us up here?

markcmiller86 commented 1 month ago

@JustinPrivitera can you elaborate here a bit on why the ghost data Mili ws providing was not sufficient?

JustinPrivitera commented 1 month ago

@JustinPrivitera can you elaborate here a bit on why the ghost data Mili ws providing was not sufficient?

@markcmiller86 yes. Mili does not provide ghost data, but it does provide a variable called sand. Sand is a mask that is always either 0 or 1 that says if a particular zone was destroyed in the simulation. We mark the destroyed zones as ghosts using the sand data.

This ghost data tells us nothing about duplicated edges across domain boundaries, which is the information we need in order to hide the edge lines correctly.

We need to leverage global node ids to get VisIt to mark ghost zones appropriately to eliminate extra edge lines which can be seen in the picture in the original ticket. VisIt won't use the global node ids to make ghost zones unless the dataset already does not have ghosts.

markcmiller86 commented 1 month ago

@JustinPrivitera can you elaborate here a bit on why the ghost data Mili ws providing was not sufficient?

@markcmiller86 yes. Mili does not provide ghost data, but it does provide a variable called sand. Sand is a mask that is always either 0 or 1 that says if a particular zone was destroyed in the simulation. We mark the destroyed zones as ghosts using the sand data.

This ghost data tells us nothing about duplicated edges across domain boundaries, which is the information we need in order to hide the edge lines correctly.

We need to leverage global node ids to get VisIt to mark ghost zones appropriately to eliminate extra edge lines which can be seen in the picture in the original ticket. VisIt won't use the global node ids to make ghost zones unless the dataset already does not have ghosts.

Ok, thanks for that description. Did you by chance look at the possibility of handling Mili sand data as "missing values"? I agree they are not ghost values and so am wondering why we are allowing Mili plugin to treat them as ghost. I think we have other mechanisms in VisIt for handling "there but should be ignored" values.

markcmiller86 commented 1 month ago

Also, do we know for sure that Mili sand functionality is even essential to support in the plugin? My recollection (from long, long ago now) was that it was an experimental feature of Mili/Griz at the time and maybe a one off. Who is the right authority to say for sure that VisIt's Mili plugin must support sand variable.

JustinPrivitera commented 1 month ago

Ok, thanks for that description. Did you by chance look at the possibility of handling Mili sand data as "missing values"? I agree they are not ghost values and so am wondering why we are allowing Mili plugin to treat them as ghost. I think we have other mechanisms in VisIt for handling "there but should be ignored" values.

Cyrus and I looked into providing the sand data as missing values, but ultimately determined that we should leave it as is. I don't remember the rationale as this has been a journey of 1-2 years.

JustinPrivitera commented 1 month ago

Also, do we know for sure that Mili sand functionality is even essential to support in the plugin? My recollection (from long, long ago now) was that it was an experimental feature of Mili/Griz at the time and maybe a one off. Who is the right authority to say for sure that VisIt's Mili plugin must support sand variable.

This I can answer definitively, having spoken to the engineering stakeholders about this quite recently. Sand is very necessary to support and removing it will have a large impact on a minority of workflows. Not all mili problems use sand but there are a significant number that do. I spoke to them about the possibility of forcing users to use threshold to get rid of sanded out portions of the mesh and they vetoed the proposal.

markcmiller86 commented 1 month ago

BTW...if sand is defined 0/1 everywhere on the mesh, why isn't the right answer a threshold operator/expression variable that just does that?

JustinPrivitera commented 1 month ago

BTW...if sand is defined 0/1 everywhere on the mesh, why isn't the right answer a threshold operator/expression variable that just does that?

Github isn't fast enough for back and forth šŸ™ƒ

Like I said:

I spoke to them about the possibility of forcing users to use threshold to get rid of sanded out portions of the mesh and they vetoed the proposal.

They told me that that would be too much of a change for folks, especially when things have just worked for years. They said that any way we can keep supporting sand as we have been is much preferred to asking users to use the threshold operator.

markcmiller86 commented 1 month ago

I spoke to them about the possibility of forcing users to use threshold to get rid of sanded out portions of the mesh and they vetoed the proposal.

:confused:...hacking on our already hugely complex ghost functionality is not really apetizing for us, either. Sorry to be uncooperative. Maybe I am not the right person to review this. I think its the wrong strategy and just complicates what ghosts mean (which now raises a proliferation risk to other plugins) and how they get computed and handled in generic db. I think the right answer is to push-back on customers. "Too much of a change". Um, sorry, VisIt is a different tool than Griz. Its going to be different. You are going to have to change practices to use. It does the same things (and more) Griz does differently. Learn to use it just like you once had to learn to use Griz and stop asking VisIt to behave just like Griz. Or, use Griz and don't bother us.

JustinPrivitera commented 1 month ago

I spoke to them about the possibility of forcing users to use threshold to get rid of sanded out portions of the mesh and they vetoed the proposal.

šŸ˜•...hacking on our already hugely complex ghost functionality is not really apetizing for us, either. Sorry to be uncooperative. Maybe I am not the right person to review this. I think its the wrong strategy and just complicates what ghosts mean (which now raises a proliferation risk to other plugins) and how they get computed and handled in generic db. I think the right answer is to push-back on customers. "Too much of a change". Um, sorry, VisIt is a different tool than Griz. Its going to be different. You are going to have to change practices to use. It does the same things (and more) Griz does differently. Learn to use it just like you once had to learn to use Griz and stop asking VisIt to behave just like Griz. Or, use Griz and don't bother us.

There's no need to apologize. I'm inclined to agree with you. I don't like that we had to hack generic db to make this happen.

It's not my call to say whether or not we should push back (or emplace back?) on customers. In this case, this is a feature that Eric, Cyrus, and myself have spoken to the engineering stakeholders about many times over the past 2 years. Eric's initial approach to solve this involved hacking ghost logic in generic db. We tried all manner of approaches in the mili plugin itself, and more recently went back to an approach that modified generic db in a minimally invasive way. Other plugins will only be affected if they provide extra ghosts arrays, and none of them do other than mili.

@cyrush has told me that we are getting this feature in for this release. This solution was something we worked on together. We can discuss more at the meeting today if you'd like, but I think that in this case, we need to make this feature work for these users.

cyrush commented 1 month ago

The current Mili implementation provides the primary mesh w/o the "sand" zones, as well as a secondary mesh that includes all the zones.

This is what the end users want -- they have been this and are mostly happy.

However, the way we are using ghost zones to do sand-based threshold this in the current implementation undermines proper ghost zone creation and breaks some things in VisIt.

We have tried several solutions over 2 years -- with restrictions on where we can do collective communication in the DB plugin vs later in Generic Database, this is a very complex problem.

VisIt can create the proper ghost zones from global node ids that are available. We trust VisIt to do this robustly.

However, VisIt will not create any ghost zones if the plugin provides them, which we were doing to "solve" to sand issue.

The current Mili plugin does not provide complete ghost info, and we can't leverage VisIt's ghost zone comm in the plugin. It has to happen later.

Missing data is not appropriate b/c it applies to values in a specific array vs using one array to screen others (yes what we want is like a threshold).

They don't want to apply a threshold every time when we can present the mesh they are after, again they know we can b/c we have been doing that with some limitations.

JustinPrivitera commented 1 month ago

Merge w/ develop in #19936