-
Notifications
You must be signed in to change notification settings - Fork 52
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
Base class for DVGeos #130
Conversation
Codecov Report
@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 62.60% 62.61% +0.01%
==========================================
Files 43 45 +2
Lines 11207 11217 +10
==========================================
+ Hits 7016 7024 +8
- Misses 4191 4193 +2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good start. Some comments below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really familiar with abstract methods and how to exploit them, but I think a couple of fixes might be needed here.
Also, pls check for more leftover comments over the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think there should be more things that we can pull out, but maybe that needs a bigger refactoring. See specific comments below
The tests are hanging. I tried cancelling the build, but Azure is slow to respond. Testing locally first would be ideal |
They were passing locally, I'll look into it later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by some of the abstract class and methods stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but I'd appreciate other diligent reviews
Purpose
This is pretty much ready for review now, the biggest lingering questions are what the new classes should be named and if there are more functions that can be brought up a level after some refactoring.
Addresses #125. There is repeated code and similar structures between the different
DVGeo
classes, resulting in issues when there are changes in the API. There is now an abstract base class,BaseDVGeo
, that allDVGeo
classes will inherit from, as well as an intermediate class,DVGeoSketch
, forDVGeoESP
andDVGeoVSP
to inherit from due to the large amount of shared code.There is also a base class for design variables now to unify the different implementations better.
Expected time until merged
Maybe a week now that it's closer to complete, there's no rush
Type of change
Testing
I do not believe any additional tests will be needed, the functionality should be the same so if the original tests pass this should be good.
Checklist
flake8
andblack
to make sure the code adheres to PEP-8 and is consistently formatted