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.

Sign up for GitHub

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

Jump to bottom

Sprint34 add 1D, 2D utility plot function #491

Merged
merged 6 commits into from
Aug 29, 2024
Merged

Sprint34 add 1D, 2D utility plot function #491

merged 6 commits into from
Aug 29, 2024

Conversation

amal-ghamdi
Copy link
Contributor

closes #489

chaozg
chaozg reviewed Aug 28, 2024
View reviewed changes
Copy link
Contributor

@chaozg chaozg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @amal-ghamdi for this quick implementation. I think it's already quite nice and I only left some tiny comments, e.g., allowing plot in log scale and displaying colorbar by default

cuqi/utilities/_utilities.py Show resolved Hide resolved
cuqi/utilities/_utilities.py Show resolved Hide resolved
cuqi/utilities/_utilities.py Show resolved Hide resolved
chaozg
chaozg reviewed Aug 28, 2024
View reviewed changes
cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
jakobsj
jakobsj approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Looks good, approving directly if you agree with very minor comments.

cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
cuqi/utilities/_utilities.py Show resolved Hide resolved
cuqi/utilities/_utilities.py Outdated Show resolved Hide resolved
cuqi/utilities/_utilities.py Show resolved Hide resolved
amal-ghamdi and others added 3 commits August 29, 2024 05:21
@amal-ghamdi @jakobsj
Update cuqi/utilities/_utilities.py
6aede3a 
Update grid point for plotting based on comment from Jakob

Co-authored-by: Jakob Sauer Jørgensen <jakobsj@users.noreply.github.com>
@amal-ghamdi
update tests
04a369a
@amal-ghamdi
add ability to plot with log scale, return matplotlib object, accept …
4d2ddb3 
…matplotlib kwargs
@amal-ghamdi
Copy link
Contributor Author

Thank you @chaozg and @jakobsj for your reviews! I addressed them and I appreciate it if you could take a second look

@jakobsj I requested your review since I handled the suggestion about colorbar differently

jakobsj
jakobsj approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Good solution to follow matplotlib style!

chaozg
chaozg approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@chaozg chaozg left a comment

Choose a reason for hiding this comment

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

LGTM

@amal-ghamdi
Copy link
Contributor Author

Many thanks, @chaozg and @jakobsj for your timely reviews and approval! I am taking the liberty to resolve the open conversations :)

@amal-ghamdi amal-ghamdi merged commit 9d89203 into main Aug 29, 2024
6 checks passed
@amal-ghamdi amal-ghamdi deleted the sprint34_add_plot_function branch August 29, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaozg chaozg chaozg approved these changes

@jakobsj jakobsj jakobsj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add plotting to utilities
3 participants
@amal-ghamdi @jakobsj @chaozg