Skip to content

Conversation

@clatapie
Copy link
Collaborator

@clatapie clatapie commented Oct 3, 2024

CICD is failing due to an error with example 28.

@clatapie clatapie added bug Something isn't working documentation Improvements or additions to documentation labels Oct 3, 2024
@clatapie clatapie self-assigned this Oct 3, 2024
@github-actions github-actions bot added the dependencies Related with project dependencies label Jan 22, 2025
@github-actions github-actions bot added the maintenance Package and maintenance related label Jan 22, 2025
@clatapie
Copy link
Collaborator Author

This PR will need a few adjustments after ansys/pymapdl#3702 will be merged.

@clatapie clatapie requested review from RobPasMue and germa89 March 18, 2025 13:43
@RobPasMue
Copy link
Member

Okay several things:

  • We need to stop mixing things in PRs. Or at least adapt the PR title and description accordingly.
    ** I have the feeling all the license header changes can be done separately. Same goes for the vale changes.
  • This PR is no longer about a single example but all of them - adapting to the plotter changes performed in PyMAPDL
  • I'll try to review as soon as I can more in detail - but I did want to give you some first feedback

@clatapie clatapie changed the title fix: example 28 fix: example rendering and general maintenance Mar 18, 2025
@clatapie clatapie changed the title fix: example rendering and general maintenance fix: example rendering Mar 18, 2025
@clatapie
Copy link
Collaborator Author

Thanks for raising this issue @RobPasMue.

This was initially the case with #252. However, the workflow was failing due to the example 28 that has been fixed by @AlejandroFernandezLuces;I needed both changes in order to have the documentation builds correctly. As the issue related to the example 28 has been fixed and merged, I can now split both PRs.

We will merge the PRs as follow:

  • maint: styling actions in CICD #252 handles maintenance fixes - as it was intended to
  • this PR fixes the plot rendering fixed by changing PyVista theme and adds each theme argument in plotters

@github-actions github-actions bot removed the dependencies Related with project dependencies label Mar 27, 2025
@clatapie
Copy link
Collaborator Author

This PR is ready to be reviewed @RobPasMue, @germa89, @AlejandroFernandezLuces!

@AlejandroFernandezLuces

I see in example 28 the overlapping faces issue, is it possible to fix this? If the example is not the issue and problem resides in the Mapdl plotter, please let me know
image

Copy link

@AlejandroFernandezLuces AlejandroFernandezLuces left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that, LGTM!

@clatapie
Copy link
Collaborator Author

From my understanding, the issue is coming from PyMAPDL - and not from the plotter. A confirmation from the PyMAPDL team would be very welcome though! Pinging @vnamdeo, @ansys/pymapdl-developers and @germa89 for visibility.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from my comment - LGTM

@germa89
Copy link
Contributor

germa89 commented Mar 27, 2025

From my understanding, the issue is coming from PyMAPDL - and not from the plotter. A confirmation from the PyMAPDL team would be very welcome though! Pinging @vnamdeo, @ansys/pymapdl-developers and @germa89 for visibility.

PyMAPDL plots the colours on the surfaces on "top" of the previous (grey) colour, I guess that two coincident surfaces plotted with different colours raise this issue, specially visible when draggin the model.

I could have a look at it but it will be on my backlog for a while. @clatapie you should open an issue on PyMAPDL repo then.

@RobPasMue
Copy link
Member

If that's the case let's merge and raise the issue in PyMAPDL

@clatapie
Copy link
Collaborator Author

I was probably not really clear with the "PyMAPDL" mention. I don't think it's an actual PyMAPDL bug.

I did some investigations on it and here is the status.

The plotter is plotting the two following elements on a unique plot:

  • The main element
mapdl.allsel("all")
mapdl.esel('s', 'mat', '', 2)
mapdl.nsle('s')

pl = mapdl.eplot(plot_bc=True, 
                    bc_glyph_size=0.002,
                    return_plotter=True,
                    show_axes=False,
                    theme=mytheme)

image

  • An additional element:
for elem, color in zip((170, 174), ('red', 'blue')):

    mapdl.esel('s', 'mat', '', 2)
    mapdl.esel("r", "ename", "", elem)
    esurf = mapdl.mesh._grid.linear_copy().extract_surface().clean()
    if mapdl.mesh.n_elem != 1:
        pl.add_mesh(
            meshes=esurf,
            show_edges=True,
            show_scalar_bar=False,
            style='surface',
            color=color
        )

image

As visible, there is an overlapping on the modeling as the top of the cylinder needs to be plotted twice at the exact same position.
IMO, this behavior could be modified on the PyMAPDL example (and is not a bug coming from PyMAPDL).
Also, I have the feeling the plotter is behaving the correct way as it might not be "normal" to plot an identical module twice with different colors.

Feel free to share your views on those points.

@RobPasMue
Copy link
Member

Agreed - we should remove the duplicate from the plotting (unless strictly necessary)

@clatapie
Copy link
Collaborator Author

After a few attempts, I have not been able to remove the duplicate plot of the face. I opened an issue (#289) to keep a track of it and merge this PR.

@RobPasMue
Copy link
Member

Let's move forward - thanks @clatapie

@RobPasMue RobPasMue merged commit f3ff7b5 into main Mar 31, 2025
11 checks passed
@RobPasMue RobPasMue deleted the fix/cicd_fail_example28 branch March 31, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation maintenance Package and maintenance related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants