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

Implemented aExp and bExp options #64

Merged
merged 3 commits into from
Feb 4, 2022
Merged

Conversation

sseraj
Copy link
Collaborator

@sseraj sseraj commented Dec 13, 2021

Purpose

Closes #40. Please see the issue for some discussion on the performance penalty compared to hard-coded values for the exponents.

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

I plan to add a test with non-default exponents in a subsequent PR where I will add a test for MultiUSMesh.

Checklist

  • 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 December 13, 2021 16:13
@sseraj sseraj requested review from SichengHe and joanibal December 13, 2021 16:13
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #64 (9618729) into master (5656b04) will decrease coverage by 1.01%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   45.80%   44.78%   -1.02%     
==========================================
  Files           6        6              
  Lines         751      748       -3     
==========================================
- Hits          344      335       -9     
- Misses        407      413       +6     
Impacted Files Coverage Δ
idwarp/UnstructuredMesh.py 60.99% <ø> (-0.04%) ⬇️
idwarp/UnstructuredMesh_C.py 46.15% <0.00%> (-53.85%) ⬇️

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 5656b04...93dc612. Read the comment docs.

Copy link
Collaborator

@joanibal joanibal 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 to me. This will be very useful.
Thank you @sseraj!

@marcomangano
Copy link
Contributor

As discussed during the maintenance meeting, a test will be added in an upcoming PR

@ewu63
Copy link
Collaborator

ewu63 commented Jan 16, 2022

@eirikurj @SichengHe could you take a look at this some time? It would be really helpful to get this feature merged.

@joanibal
Copy link
Collaborator

@SichengHe @eirikurj. Just another reminder, as I would also like to get this feature on the main branch ASAP

@sseraj sseraj requested review from awccopp and removed request for SichengHe February 4, 2022 16:33
Copy link

@awccopp awccopp left a comment

Choose a reason for hiding this comment

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

Everything here looks good to me.

@sseraj sseraj merged commit 4a8a6ed into mdolab:master Feb 4, 2022
@sseraj sseraj deleted the exp-options branch February 4, 2022 23:09
@sseraj sseraj mentioned this pull request Feb 18, 2022
12 tasks
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.

Implement the aExp and bExp options
5 participants