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

ENH: disp more results to rocketpy.plots & rocketpy.prints #753

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mohdsaid497566
Copy link

@mohdsaid497566 mohdsaid497566 commented Dec 14, 2024

See issue 730

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

NA

New behavior

NA

Breaking change

  • No

@mohdsaid497566 mohdsaid497566 requested a review from a team as a code owner December 14, 2024 08:55
@Gui-FernandesBR Gui-FernandesBR force-pushed the enh/more-monte-carlo-results branch from 2e99e7f to 2c2641d Compare December 15, 2024 02:53
Comment on lines +31 to +34
pt = self.monte_carlo.results[key]
print (f"{key:>25} {value[0]:>15.3f} {value[1]:>15.3f} {np.quantile(pt,0):>15.3f} {np.quantile(pt,0.025):>15.3f} {np.quantile(pt,0.5):>15.3f} {np.quantile(pt,0.975):>15.3f} {np.quantile(pt,1):>15.3f}")
except TypeError:
print(f"{key:>25} {str(value[0]):>15} {str(value[1]):>15}")
print (f"{key:>25} {str(value[0]):>15} {str(value[1]):>15} {str(np.quantile(pt,0)):>15} {str(np.quantile(pt,0.025)):>15} {str(np.quantile(pt,0.5)):>15} {str(np.quantile(pt,0.975)):>15} {str(np.quantile(pt,1)):>15}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe "min" and "max" are way more clear than "0% quantile" and "100% quantile".

Comment on lines +330 to +333
],
"files.associations": {
"plyconfig.json": "jsonc"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the issue.

Comment on lines +179 to +194
figure, plt = plt.subplots(3,1,sharex=True,gridspec_kw={'height_ratios':[1,3]})

plt[0].boxplot(self.monte_carlo.results[key],vert=False)
plt[0].ytick([])

plt[1].hist(self.monte_carlo.results[key])
plt[1].title(f"Histogram of {key}")
plt[1].ylabel("Number of Occurrences")

plt[2].hist(self.monte_carlo.results[key], density=True)
plt[2].title(f" Density {key}")
plt[2].ylabel("Probability Density")
kde = kde.gaussian_kde(self.monte_carlo.results[key])
x_array = np.linspace(min(self.monte_carlo.results[key]), max(self.monte_carlo.results[key]), 100)
plt[2].plot(x_array, kde(x_array), label='KDE')
plt.show()
Copy link
Contributor

Choose a reason for hiding this comment

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

This generates errors when I try to create plot MonteCarlo results. Indeed, you are replacing a global variable, the alias plt for the submodule matplotlib.pyplot for a local variable with the same name. Moreover, where does kde come from? It seems you have not imported it. Finally, I tried to do a quick clean of the errors just to get a gist of how it would look, and it seems that it creates three different plots. As mentioned in the PR, the idea was to create a single plot that blends the three (histogram, boxplot and density plot).

@Gui-FernandesBR Gui-FernandesBR force-pushed the enh/more-monte-carlo-results branch from 2c2641d to cdbf376 Compare December 16, 2024 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants