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

C++ modules should start using setters and getters #715

Closed
schaubh opened this issue Jun 6, 2024 · 3 comments · Fixed by #716
Closed

C++ modules should start using setters and getters #715

schaubh opened this issue Jun 6, 2024 · 3 comments · Fixed by #716
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@schaubh
Copy link
Contributor

schaubh commented Jun 6, 2024

Describe your use case
C++ Basilisk modules sometimes have user configurable variable foo defined as a public variable set, and thus set through

mod.foo = 1.0

Some modules use setters and getters where the module variable is set using

mod.setFoo(1.0)

Describe alternatives solutions you've considered
Going forward we should have all new C++ Basilisk module variables be private and user setter and
getter methods. The setter method should check if the variable is being given valid number(s). Required variables
can still be checked in the module Reset() method. This way the python code with throw an immediate error if a
variable is set to an invalid value.

Further, if the use of a variable is being depreciated, the associated setter and getter methods can be gracefully
depreciated. If the variables are public class variables, there is now way to catch all the ways this variable could be set.

Additional context
All the instructions need to be updated to discuss using setters and getters in C++ modules, the makeDraftModule.py
script needs to be updated, the C++ template modules need updating. Further, with private variables it the regular
module variable logger cannot be used. Documentation needs to be updated on how to log a private class variable.

@schaubh schaubh self-assigned this Jun 6, 2024
@schaubh schaubh added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 6, 2024
@schaubh
Copy link
Contributor Author

schaubh commented Jun 6, 2024

@juan-g-bonilla , I just pushed my setter/getter branch to feature/bsk_715_setterGetter. I'm able to log the private variables now with the get method. However, I wonder if we should make a new utility function in sys_model.i called loggerGet() that logs specifically a variable foo with the method getFoo()? If setters and getters are becoming the norm, then I feel we should make this logging process easier.

Or, can we expand the logger() functionality to see if the variable can be accessed via mod.foo. If not, then try using mod.getFoo(). If that doesn't work, then throw the current error message? That way the logging interface for public variables and getter accessible private variables remains the same?

@juan-g-bonilla
Copy link
Contributor

bsk_715_setterGetter

I see the issue now. The thing is that because we are using raw C arrays, we need to return them from functions as pointers:

double * getFoo();

but then SWIG does not have enough information to transform that into a Python type (because it doesn't know the length of the data). There is really no way to return something like this in C++:

double[3] getFoo();

So we would need to use a more information rich type. I've added some commits with a switch to using std::array<double, 3> and SWIG changes to support that. I've also updated .logger with your idea about automatically finding getters, and it seems to work pretty good. As long as the documentation explains what's going on, I think it's a good change. Feel free to reset and play around with my changes.

Changing from C arrays to std::array might require some code churn, but I think it's step in the right direction anyway. Of course C arrays will still be necessary in C messages, but intermixing C arrays and std::array is pretty painless.

@schaubh
Copy link
Contributor Author

schaubh commented Jun 7, 2024

Thanks for the support @juan-g-bonilla . I pulled your commits to my branch and will review and integrated. I like the new combined logger() functionality. I think this is a clean solution for the user. I need to update documentation to reflect this, and still keep the explanation on how to do more complex logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants