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

Name input #60

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Name input #60

wants to merge 3 commits into from

Conversation

ArshSaja
Copy link
Contributor

Purpose

This PR adds an option to combining grids. If one of the block name is based on "domain", if it needs to be set to a specific name, then it can be added as an option.

Expected time until merged

Type of change

  • 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

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • 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

@ArshSaja ArshSaja requested a review from a team as a code owner September 26, 2022 20:07
@ArshSaja ArshSaja requested review from eirikurj, akleb and sseraj and removed request for eirikurj September 26, 2022 20:07
@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #60 (92807ef) into main (a07a21a) will increase coverage by 0.02%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   21.89%   21.92%   +0.02%     
==========================================
  Files           3        3              
  Lines        2279     2281       +2     
==========================================
+ Hits          499      500       +1     
- Misses       1780     1781       +1     
Impacted Files Coverage Δ
cgnsutilities/cgnsutilities.py 27.81% <75.00%> (+0.02%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@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.

Is the idea to use this when combining nearfield and background meshes from simpleOCart? This approach would work for that case. However, if I wanted to combine two or more grids that all had blocks originally named "domain", I could not use this approach to give each grid a different name.

I think renaming blocks would be better as a preprocessing step before combineGrids. My suggestion is to add a kwarg to renameBlocks like newName="domain". If useOldNames=False, the blocks are renamed as newName.##### This way, we can use renameBlocks to give each grid its desired name before calling combineGrids.

@eirikurj
Copy link
Contributor

@ArshSaja whats the status here, do you plan to address @sseraj comments?

@ArshSaja
Copy link
Contributor Author

Yeah, I will address his comments sooner, I haven't worked on this for a while. But once my current tasks are done, I will go through this.

@eirikurj
Copy link
Contributor

eirikurj commented Mar 6, 2023

@ArshSaja whats the status on this?

@ArshSaja
Copy link
Contributor Author

ArshSaja commented Mar 6, 2023

Sorry for the late updates @eirikurj . I will change it to a draft for now. I will provide more details on that later.

@ArshSaja ArshSaja marked this pull request as draft March 6, 2023 16:06
@eirikurj
Copy link
Contributor

@ArshSaja any updates on this?

@ArshSaja
Copy link
Contributor Author

@ArshSaja any updates on this?

I haven't looked at it for a while. I will try to check this when I am done with the other pending PRs.

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.

3 participants