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.

Sign up for GitHub

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

Jump to bottom

FIx omegah tests #996

Merged
merged 3 commits into from
Oct 17, 2023
Merged

FIx omegah tests #996

merged 3 commits into from
Oct 17, 2023

Conversation

bartgol
Copy link
Collaborator

@bartgol bartgol commented Oct 17, 2023

Fix calculation of ownership array in OmegahConnMgr, and fix a couple of bugs in box mesh and its tests.

Closes #995

@bartgol bartgol added bugfix Omega_h Topics related to the Omega_h mesh library labels Oct 17, 2023
@bartgol bartgol requested a review from cwsmith October 17, 2023 18:27
@bartgol bartgol self-assigned this Oct 17, 2023
@ikalash
Copy link
Collaborator

ikalash commented Oct 17, 2023

I just tested this and am still seeing 2 test failures:

  5 - disc_omegah_box_mesh_UnitTest_Serial (Failed)
  6 - disc_omegah_box_mesh_UnitTest_Parallel (Failed)

Do they pass for you?

@bartgol
Copy link
Collaborator Author

bartgol commented Oct 17, 2023

Ah, thanks, I forgot to commit the last changes.

@ikalash
Copy link
Collaborator

ikalash commented Oct 17, 2023

Ah ok. I will pull / retest once you push them.

@bartgol
Copy link
Collaborator Author

bartgol commented Oct 17, 2023

Uhm, I pushed everything. It was passing earlier, and now it doesn't. I must have messed up while committing changes. I'll fix it soon.

@ikalash
Copy link
Collaborator

ikalash commented Oct 17, 2023

Ok, thank you. Let me know when it's ready to test again.

@bartgol
Copy link
Collaborator Author

bartgol commented Oct 17, 2023

Ok, they pass on my laptop now.

@ikalash
Copy link
Collaborator

ikalash commented Oct 17, 2023

Crap... I approved / merged the wrong PR. This one is good - all tests pass now. I just tested.

@ikalash
Copy link
Collaborator

ikalash commented Oct 17, 2023

We can see what happens with the other one tomorrow since it's merged. Sorry.... !

@ikalash ikalash merged commit a44f55c into master Oct 17, 2023
@ikalash ikalash deleted the bartgol/omegah-fixes branch October 17, 2023 21:52
@bartgol
Copy link
Collaborator Author

bartgol commented Oct 17, 2023

No problem!

bartgol
bartgol commented Oct 18, 2023
View reviewed changes
src/disc/omegah/OmegahConnManager.cpp
gdn[i] = createGlobalEntDofNumbering(mesh, i, m_dofsPerEnt[i], startingOffset);
const auto offset = getMaxGlobalEntDofId(mesh, gdn[i]);
startingOffset = offset == 0 ? startingOffset : offset;
gdn[i] = createGlobalEntDofNumbering(mesh, i, m_dofsPerEnt[i], offset);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cwsmith here

cwsmith
cwsmith reviewed Oct 18, 2023
View reviewed changes
src/disc/omegah/OmegahConnManager.cpp
const auto offset = getMaxGlobalEntDofId(mesh, gdn[i]);
startingOffset = offset == 0 ? startingOffset : offset;
gdn[i] = createGlobalEntDofNumbering(mesh, i, m_dofsPerEnt[i], offset);
offset += getMaxGlobalEntDofId(mesh, gdn[i])*m_dofsPerEnt[i];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit concerned that including *m_dofsPerEnt[i] will count the dofs per ent twice. I need to check the implementation, but I'm pretty sure that the global dof numbers (gdn[0:4]) produced by createGlobalEntDofNumbering(...) will already include this factor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch! I will remove this counter then. To be clear, it's not "wrong", but it may make GIDs unnecessarily large, right?

Copy link
Collaborator

@cwsmith cwsmith Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I will remove this counter then.

Sounds good. Thank you.

To be clear, it's not "wrong", but it may make GIDs unnecessarily large, right?

Yeah. Exactly.

For the record, 'dofsPerEnt' is included in the global Id computation here:

Albany/src/disc/omegah/OmegahConnManager.cpp

Line 171 in e13e9e0

const auto dofGlobalId = entGlobalIds[i]*dofsPerEnt+j;
.

Looking at this line:

Albany/src/disc/omegah/OmegahConnManager.cpp

Line 172 in e13e9e0

dofNum[dofIndex] = startingOffset+dofGlobalId;

using offset += getMaxGlobalEntDofId(...) may also be padding the offset unnecessarily.

For example, suppose offset=100 is passed to createGlobalEntDofNumbering(...), and, for this example, creates 20 dof ids. We then call getMaxGlobalEntDofId(...) and that returns 120. If we accumulated into offset then we would have 220 as the offset for the next iteration instead of 120.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you're right. I think I confused dofs numbering with geo entities numbering. I will roll back these changes.

cwsmith added a commit to SCOREC/Albany that referenced this pull request Oct 20, 2023
@cwsmith
Revert "Merge pull request sandialabs#996 from sandialabs/bartgol/ome…
6adfca6 
…gah-fixes"

This reverts commit a44f55c, reversing
changes made to bcc7a2b.

Conflicts:
	src/disc/omegah/OmegahConnManager.cpp
	src/disc/omegah/OmegahConnManager.hpp
cwsmith added a commit to SCOREC/Albany that referenced this pull request Oct 23, 2023
@cwsmith
Revert "Merge pull request sandialabs#996 from sandialabs/bartgol/ome…
1c8254c 
…gah-fixes"

This reverts commit a44f55c, reversing
changes made to bcc7a2b.

Conflicts:
	src/disc/omegah/OmegahConnManager.cpp
	src/disc/omegah/OmegahConnManager.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwsmith cwsmith cwsmith left review comments

Assignees

@bartgol bartgol

Labels
bugfix Omega_h Topics related to the Omega_h mesh library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failing omegah tests
3 participants
@bartgol @ikalash @cwsmith