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

Added dpi and savepdf options to comparison report for publications #123

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

ledm
Copy link
Collaborator

@ledm ledm commented Dec 14, 2023

Colin asked for a higher resolution version of the images, so I added an option is the report maker to set image resolution in dpi, and also a flag to save images as a pdf.

bgcval2/bgcval2_make_report.py Outdated Show resolved Hide resolved
@ledm ledm requested a review from valeriupredoi December 14, 2023 14:11
@ledm
Copy link
Collaborator Author

ledm commented Dec 14, 2023

I'm definitely getting my money's worth out of @valeriupredoi this week! Thanks for the hard work mate!

@valeriupredoi
Copy link
Owner

only that you're not paying me a single penny though 🤣 Lemme have a looksee in a jiffy 👍

Copy link
Owner

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

fine by me, but I'd add a default value for dpi, and I'd also add a check on how dpi is specified by the user - if it's not like eg dpi=200 and it's dpi=200x300 that should probably be caught before Matplotlib throws a hissy (basically try: int(dpi) else: raise ValueError("that need be an int you dummy")) - up to you

@ledm
Copy link
Collaborator Author

ledm commented Dec 14, 2023

If we leave the dpi as the default value (None) then the image will be saved using the user's default dpi settings, which can be changes using:

plt.rcParams['savefig.dpi'] = 300

@valeriupredoi
Copy link
Owner

okidoki 👍

@ledm
Copy link
Collaborator Author

ledm commented Dec 14, 2023

Added a int flag check for dpi.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@ledm
Copy link
Collaborator Author

ledm commented Dec 14, 2023

Done here. Going to merge if V is happy.

Copy link
Owner

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

good stuff! Fire at will, bud 🍺

@ledm ledm merged commit e389ba0 into main Dec 14, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants