🤖 Bug fix for real attributes in parallel socket input (glvis -a)

[tzanio]

Disclaimer

The initial versions of this PR were written with a coding agent 🤖, however:

• I have reviewed, edited and tested all changes before issuing the PR
• I claim to understand the code and pledge to maintain it in the future

Overview

This is a bug fix for glvis -a which did not work correctly for parallel socket input.

Testing

Start the server with

glvis -a

then send a solution via socket, from a mesh with multiple attributes, e.g.

mpirun -np 4 ./ex1p -m ../data/beam-quad.mesh

and check that F11 pulls apart material subdomains, not processor subdomains.

Potential improvements

• Add the ability to specify this via a socket command?

[Copilot]

Pull request overview

Fixes glvis -a behavior for parallel socket input in server mode by ensuring the “real attributes” setting is propagated into per-connection visualization sessions (so material attributes are preserved instead of being overwritten with processor ranks).

Changes:

• Thread the keep_attr (-a/--real-attributes) flag through GLVisServer(...) and into each new Session.
• Initialize Session windows with win.data_state.keep_attr so StreamReader preserves real mesh attributes for parallel socket streams.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[najlkin]

Looks good 👍 . It has been broken for a long time and the refactoring fixed that for other places, but even the parameters were missing here, so I was wondering if the bug is not a feature 😅.
We have a script command for that (psolution), but stream commands work differently and each part is sent with just solution, so we cannot just add easily a parameter there. Perhaps we could add some commands which would go before solution? 🤔 But we do not have that for other things and it requires to change how things are constructed and processed. Right now, StreamReader reads the stream and initializes DataState from which the visualization is initialized and only then processing is passed to communication_thread, which processes the rest. If we do not want change this completely, we could think of adding these initialization commands to StreamReader and let it process as many as needed until it reaches some solution command 🤔

[najlkin]

The changes for stream commands for keeping attributes and other things are in #369 .

[tzanio]

Looks good 👍 . It has been broken for a long time and the refactoring fixed that for other places, but even the parameters were missing here, so I was wondering if the bug is not a feature 😅.

I think it used to work before e6c40a8, but I could be wrong.

We have a script command for that (psolution), but stream commands work differently and each part is sent with just solution, so we cannot just add easily a parameter there. Perhaps we could add some commands which would go before solution? 🤔 But we do not have that for other things and it requires to change how things are constructed and processed. Right now, StreamReader reads the stream and initializes DataState from which the visualization is initialized and only then processing is passed to communication_thread, which processes the rest. If we do not want change this completely, we could think of adding these initialization commands to StreamReader and let it process as many as needed until it reaches some solution command 🤔

Just to confirm: the above paragraph is about the potential improvement suggestion to support this via a socket command?

[tzanio]

The changes for stream commands for keeping attributes and other things are in #369 .

Do I understand correctly that you are proposing that we merge #369 here first?

I was not sure if we want that or not... @v-dobrev, what do you think?

[najlkin]

Looks good 👍 . It has been broken for a long time and the refactoring fixed that for other places, but even the parameters were missing here, so I was wondering if the bug is not a feature 😅.

I think it used to work before e6c40a8, but I could be wrong.

It goes a way longer before that, I just checked it worked the last time in v4.2, where it used to be a global variable, so it got broken with moving these methods to StreamState (StreamReader today) probably.

We have a script command for that (psolution), but stream commands work differently and each part is sent with just solution, so we cannot just add easily a parameter there. Perhaps we could add some commands which would go before solution? 🤔 But we do not have that for other things and it requires to change how things are constructed and processed. Right now, StreamReader reads the stream and initializes DataState from which the visualization is initialized and only then processing is passed to communication_thread, which processes the rest. If we do not want change this completely, we could think of adding these initialization commands to StreamReader and let it process as many as needed until it reaches some solution command 🤔

Just to confirm: the above paragraph is about the potential improvement suggestion to support this via a socket command?

Yes, basically describes the reasoning behind #369 . It can be merged here or separately, it is not inter-dependent.

[dylan-copeland]

Thanks @tzanio! I tested, and everything looks fine. My only suggestion is to improve the documentation for the implementation of this functionality. It looks like keep_attr controls whether DataState::SetGridFunction calls ComputeDofsOffsets, and it is not clear to me why this would determine whether F11 splits the domain by attribute vs. parallel partition. Documentation could be added for keep_attr and these functions, as well as how they determine the behavior of F11.

[najlkin]

@dylan-copeland , ComputeDofsOffsets is newly used (#339 ) for DOFs numbering. It depends on the attributes just coincidentally in the sense that the implementation needs to know from what rank the DOFs are coming from, where @camierjs used the attribute when it is not a real one but the MPI rank. It is a detail, but it is true it could be mentioned somewhere in documentation.
That F11 pulls apart subdomains of different attributes whatever they represent. README is a bit misleading here, because it says "material subdomains", which is not always the case. This could be clarified.