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

Added function to get block info #43

Merged
merged 12 commits into from
Aug 11, 2021
Merged

Added function to get block info #43

merged 12 commits into from
Aug 11, 2021

Conversation

sseraj
Copy link
Collaborator

@sseraj sseraj commented Jul 25, 2021

Purpose

I made a few changes mostly related to retrieving block information in Python:

  • added a function called getBlockInfo that returns a dictionary of the block information
  • refactored printBlockInfo to use getBlockInfo
  • added a test for getBlockInfo
  • converted some other tests to use BaseRegTest

Related PR on pyHyp: mdolab/pyhyp#52

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Explain the steps needed to test the new code to verify that it does indeed address the issue and produce the expected behavior.

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@sseraj sseraj requested a review from a team as a code owner July 25, 2021 19:22
marcomangano
marcomangano previously approved these changes Jul 26, 2021
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Looks good! Just to double check, did you verify that the block info from the reference mesh is consistent with the grid file?

@sseraj
Copy link
Collaborator Author

sseraj commented Jul 26, 2021

Looks good! Just to double check, did you verify that the block info from the reference mesh is consistent with the grid file?

I had checked that the block info after these changes are the same as before the changes. I now just counted the block dimensions in Tecplot and the number themselves look correct

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@cc29c74). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head cf97c26 differs from pull request most recent head ceb1928. Consider uploading reports for the commit ceb1928 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master      #43   +/-   ##
=========================================
  Coverage          ?   12.76%           
=========================================
  Files             ?        3           
  Lines             ?     2202           
  Branches          ?        0           
=========================================
  Hits              ?      281           
  Misses            ?     1921           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc29c74...ceb1928. Read the comment docs.

marcomangano
marcomangano previously approved these changes Jul 26, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Jul 27, 2021

@joanibal could you take a look when you have a chance? There are 2 other PRs waiting on this so it'd be good to get this in.

Copy link
Contributor

@eirikurj eirikurj left a comment

Choose a reason for hiding this comment

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

Overall, this is fine, just a few comments. Based on the response, I might push some minor updates if that is ok.
@nwu63 mentioned that there are couples of PRs waiting on this, but there is only one linked here. What is the second one?

cgnsutilities/cgnsutilities.py Outdated Show resolved Hide resolved
tests/test_cgnsutilities.py Show resolved Hide resolved
cgnsutilities/cgnsutilities.py Show resolved Hide resolved
@ewu63
Copy link
Collaborator

ewu63 commented Jul 29, 2021

@nwu63 mentioned that there are couples of PRs waiting on this, but there is only one linked here. What is the second one?

The second one is mdolab/pyhyp#48, which will finally show coverage after we switch to using BaseRegTest (and remove the os.system calls).

Copy link
Collaborator Author

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Feel free to push changes. I think we should converge on the pyHyp tests before merging this though in case any other changes have to be made.

cgnsutilities/cgnsutilities.py Outdated Show resolved Hide resolved
cgnsutilities/cgnsutilities.py Show resolved Hide resolved
def train_getTotalCellsNodes(self):
self.test_getTotalCellsNodes(train=True)

def test_getWallCellsNodes(self, train=False):
Copy link
Collaborator

@joanibal joanibal Jul 30, 2021

Choose a reason for hiding this comment

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

I'm not a fan of this pattern. I'd rather have just one test, but that seem like a bigger issue than just this PR

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 it makes sense to have separate tests for each function because then it is easy to see what is broken if the test fails. I don't like having a separate train function and ref file for each test though.

joanibal
joanibal previously approved these changes Jul 30, 2021
@ewu63
Copy link
Collaborator

ewu63 commented Aug 3, 2021

What's left to be done here @sseraj?

@sseraj
Copy link
Collaborator Author

sseraj commented Aug 3, 2021

What's left to be done here @sseraj?

I'm converting this to a draft until we settle on how to test pyHyp. If we decide on using cgnsdiff then this can be merged after I make the minors edits suggested above. (The function won't end up being used in the pyHyp tests). Otherwise, we may want to add other utilities for the pyHyp tests like a sum over the coordinates in each direction as you suggested.

@sseraj sseraj marked this pull request as draft August 3, 2021 20:01
@sseraj sseraj marked this pull request as ready for review August 8, 2021 19:09
@sseraj
Copy link
Collaborator Author

sseraj commented Aug 8, 2021

We decided not to use this in the pyHyp tests, but I think it's worth merging anyway.

@eirikurj
Copy link
Contributor

eirikurj commented Aug 9, 2021

@sseraj, ok sounds good. I will push few changes.

@joanibal joanibal self-requested a review August 9, 2021 22:33
joanibal
joanibal previously approved these changes Aug 9, 2021
@ewu63 ewu63 merged commit b9a4e3e into mdolab:master Aug 11, 2021
@sseraj sseraj deleted the blockInfo branch August 11, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants