mdolab / CMPLXFOIL

GNU General Public License v2.0
29 stars 20 forks source link

Add aero solver interface #14

Closed A-CGray closed 2 years ago

A-CGray commented 2 years ago

Purpose

This PR adds an aero solver API for pyXLIGHT that allows it to be used in a similar manner to ADflow.

It also renames pyXLIGHT to CMPLXFOIL (let me know what needs to be changed to fix the automated testing, docs, github repo, and other maintenance stuff).

https://github.com/mdolab/pygeo/pull/141 needs to be merged first because some new CMPLXFOIL tests use DVGeometryCST.

TODO's before this can be merged:

Type of change

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

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.

A-CGray commented 2 years ago

Presumably we should bump the version with this PR too? Should we go to 1.2 or 2.0?

eytanadler commented 2 years ago

Presumably we should bump the version with this PR too? Should we go to 1.2 or 2.0?

I'd argue for 2.0 since it's an entirely new (breaking) interface. Breaking if we remove the old class, which Bernardo was suggesting.

ewu63 commented 2 years ago

I will review this by the end of this week.

codecov[bot] commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@383cf16). Click here to learn what that means. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #14   +/-   ##
=======================================
  Coverage        ?   78.36%           
=======================================
  Files           ?        4           
  Lines           ?      564           
  Branches        ?        0           
=======================================
  Hits            ?      442           
  Misses          ?      122           
  Partials        ?        0           

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ewu63 commented 2 years ago

I will most likely not have time to review this until possibly July. I'm assuming this is not blocking anyone right?

eytanadler commented 2 years ago

I will most likely not have time to review this until possibly July. I'm assuming this is not blocking anyone right?

No I don't think it'll be blocking anyone, but if it's too much we can have someone else review it instead!

bernardopacini commented 2 years ago

I have not reviewed the PR yet, but with regards to the test failing, are we going to rename the actual repository too? I would guess that Azure needs a "reset" to relink with the new URL. I think we have to delete the old pipeline and make a new one.

Unless someone disagrees, we don't have much to lose doing that through this PR.

eytanadler commented 2 years ago

I have not reviewed the PR yet, but with regards to the test failing, are we going to rename the actual repository too? I would guess that Azure needs a "reset" to relink with the new URL. I think we have to delete the old pipeline and make a new one.

Unless someone disagrees, we don't have much to lose doing that through this PR.

I think renaming the repo on GitHub makes sense. Let's give it a shot