Skip to content

ENH: add " ' " to Function __repr__ #358

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

Merged
merged 3 commits into from
May 8, 2023
Merged

Conversation

MateusStano
Copy link
Member

Pull request checklist

  • Tests for the changes have been added
  • Docs have been reviewed and added / updated if needed
  • Lint (black rocketpy) has passed locally and any fixes were made
  • All tests (pytest --runslow) have passed locally

Discussion

There is a problem in the Dispersion class when it comes to saving Function objects in the .txt files. This is a special problem in the .disp_inputs.txt files, since the values of powerOffDrag, powerOnDrag and others are received as Functions.

What happens in these cases is, when saving in the .txt file we simply pass a dictionary, which contains the info of the simulation inputs, into the write function. The interaction between the write function and the Function class is so that what gets saved in the file is the Function __repr__. For example:

 { ... "powerOffDrag": Function from R1 to R1 : (Scalar) → (Scalar), ... }

Further on, in the dispersion class, there is an attribute called inputs_log that generates, from the input file, a list in which each item is a line of the file. A list of dictionaries with all the inputs necessary to recreate any iteration of the simulation.

Now, two problems arise from this:

  • The way Functions are saved is not "redefinable", so we can't recreate that Function object just with its __repr__ information. I believe the only way to do this (from what I know) would be to start using pickle. Unfortunately, that is out of the scope of v1.0 since it would take some extra effort to implement
  • When the inputs_log is created, it can not interpret each line of the file because Function from R1 to R1 : (Scalar) → (Scalar) is not a recognizable Python object. It is not a string.

The temporary solution I found for this was to just change add a ' as the first and last character of the __repr__ string. This way, it gets saved in the .txt as string and can be read by as a python object:

 { ... "powerOffDrag": 'Function from R1 to R1 : (Scalar) → (Scalar)', ... }

I am creating this PR to both ask if there is any better way to do this, or if this is an okay solution to the problem

Does this introduce a breaking change?

  • Yes
  • No

@MateusStano MateusStano added the Monte Carlo Monte Carlo and related contents label Apr 27, 2023
@MateusStano MateusStano added this to the Release v1.0.0 milestone Apr 27, 2023
@MateusStano MateusStano self-assigned this Apr 27, 2023
Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

I don't see problem in making the inputs and outputs of the Function a "string of a string" per se, however that is a bit of a weird change since it alters the behavior of all Function instances so as meet the requirements of Dispersion. Perhaps it could also be achieved with an adapter method at the Dispersion class.

Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

As suggested, the repr() of a string was used to keep the Python pattern.

@giovaniceotto giovaniceotto merged commit db66aac into beta/v1.0.0 May 8, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the enh/function-'-repr branch May 26, 2023 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monte Carlo Monte Carlo and related contents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants