mdolab / pygeo

pyGeo provides geometric design variables and constraints suitable for gradient-based optimization.
https://mdolab-pygeo.readthedocs-hosted.com/en/latest/?badge=latest
Apache License 2.0
131 stars 55 forks source link

Adding ESP into MPhys wrapper #155

Closed hajdik closed 2 years ago

hajdik commented 2 years ago

Purpose

@bernardopacini and I added functions needed for DVGeoESP into the MPhys wrapper. Checks were also added to ensure the correct DVGeo type has been instantiated when using wrapper functions that are only for specific types, which fixes the DVGeoVSP part of the wrapper because the previous addition of child FFD functions assumed all DVGeos can use child FFDs.

Expected time until merged

1 week unless we find something else this breaks

Type of change

Testing

This works with DAFoam and ADflow ESP-MPhys scripts. If I have time I'll add a test

Checklist

codecov[bot] commented 2 years ago

Codecov Report

Merging #155 (3752929) into main (5898192) will decrease coverage by 0.14%. The diff coverage is 2.27%.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   63.90%   63.75%   -0.15%     
==========================================
  Files          47       47              
  Lines       11757    11786      +29     
==========================================
+ Hits         7513     7514       +1     
- Misses       4244     4272      +28     
Impacted Files Coverage Δ
pygeo/mphys/mphys_dvgeo.py 0.00% <0.00%> (ø)
pygeo/parameterization/DVGeoESP.py 64.88% <100.00%> (+0.04%) :arrow_up:

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

hajdik commented 2 years ago

Maybe we can modify the input arguments, have a single file input, and then determine the geo type based on the file extension?

That would work for ESP, they should all be .csm

It's a bit tricky since I feel people use different extensions for the FFD files.

Is there a set of allowable extensions for FFDs we could look for? Or if .vsp3 or .csm aren't used, it could default to using FFD.

bernardopacini commented 2 years ago

Maybe we can modify the input arguments, have a single file input, and then determine the geo type based on the file extension?

That would work for ESP, they should all be .csm

It's a bit tricky since I feel people use different extensions for the FFD files.

Is there a set of allowable extensions for FFDs we could look for? Or if .vsp3 or .csm aren't used, it could default to using FFD.

This makes me a bit nervous because even though the extension identifies the file, it isn't technically necessary to read in the file (in any format, I believe). A user might write a file without an extension and then not understand why it doesn't load into pyGeo.

Would having an option for "geo_type" be a reasonable compromise?

anilyil commented 2 years ago

Yeah, explicitly selecting geo_type also sounds good to me. Just a bit more explicit. Final decision is up to you, just raised that minor issue. I can approve the PR as is now as well.

bernardopacini commented 2 years ago

Yeah, explicitly selecting geo_type also sounds good to me. Just a bit more explicit. Final decision is up to you, just raised that minor issue. I can approve the PR as is now as well.

Sounds good to me, if it's okay with @hajdik. I like the idea of having a single option for file path / name and then specifying the geo type. Thanks for bringing this up!

hajdik commented 2 years ago

That sounds good to me, I can add that option in.

bernardopacini commented 2 years ago

That sounds good to me, I can add that option in.

On that note, could we also just make an “options” option instead of having the ESP, VSP, and FFD ones separate?

anilyil commented 2 years ago

I would prefer just using individual openmdao options for each option instead of one option taking in a dictionary. Is that what you meant?

bernardopacini commented 2 years ago

I would prefer just using individual openmdao options for each option instead of one option taking in a dictionary. Is that what you meant?

The options are different for each parametrization type, so I think we'd end up with a lot of options (and many combinations that are incompatible). I do like the idea of trying to map them directly without having a big dictionary as we have now, but I worry that might end up being many, many options listed in the wrapper?

anilyil commented 2 years ago

Now I see what you mean. so you are referring to the additional (optional) input arguments to initialize different classes? I am fine with adding that as a single option dictionary.

bernardopacini commented 2 years ago

Yes, exactly. We should combine these:

       self.options.declare("vsp_options", default=None)
       self.options.declare("esp_options", default=None)
bernardopacini commented 2 years ago

Just some minor comments on the code. Are you sure you just want to pass in the options for the different kinds of parametrizations as a dict (?), without any check before initializing the class? I assume you can rely on error messages when __init__ is run, but could be confusing in the long term imho

Regardless of if they are passed as dicts corresponding to the parametrization or as a general dict, there is no actual check for the options lining up in MPhys. So, the user could make the mistake regardless.

A proper fix for this would be to check in the __init__ function of the respective parametrization if that option in the dictionary is part of the pyGeo object. One approach is by popping the kwarg key each time a value is used and checking if the dictionary is empty at the end of the function. @hajdik is this something you'd be willing to implement or would you like me to?

Edit: this opens a fairly large can of worms as we really should be doing this for all our __init__ functions. Maybe it would be worth adding this as a separate PR since the single dictionary proposed here is not any less safe than the previous MPhys implementation?

marcomangano commented 2 years ago

Edit: this opens a fairly large can of worms as we really should be doing this for all our __init__ functions. Maybe it would be worth adding this as a separate PR since the single dictionary proposed here is not any less safe than the previous MPhys implementation?

Agreed, and as I mentioned the error messages should be already clear enough for the user

hajdik commented 2 years ago

Maybe it would be worth adding this as a separate PR since the single dictionary proposed here is not any less safe than the previous MPhys implementation?

I think that should be a separate PR with wider discussion on the DVGeo modules themselves and this can stay as just touching the MPhys wrapper with the bugfixes for VSP + adding in ESP.

bernardopacini commented 2 years ago

Edit: this opens a fairly large can of worms as we really should be doing this for all our __init__ functions. Maybe it would be worth adding this as a separate PR since the single dictionary proposed here is not any less safe than the previous MPhys implementation?

Agreed, and as I mentioned the error messages should be already clear enough for the user

I think there is a chance that a user specifies an option that is invalid and does not receive an error message. But, this could happen whenever instantiating pyGeo so this MPhys version is not any worse than before. I certainly agree with you that it is an issue we should address (though not on this PR...).

bernardopacini commented 2 years ago

This is minor, but while we are making backwards incompatible changes: can we remove geo_ from the options? Since it is the pyGeo object it should already be clear that this pertains to geometry. Then we would just have file, type, options. What do you all think?