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

Add example as a notebook: example_na_controlling_the_pml #217

Merged
merged 9 commits into from
Jan 16, 2024

Conversation

faridyagubbayli
Copy link
Collaborator

@faridyagubbayli faridyagubbayli commented Oct 8, 2023

Adds our first Colab-runnable example 🎉

Migrates an example (example_na_controlling_the_pml) from Matlab to Python as a Jupyter notebook. Changes also include a demonstration of equality between Matlab and Python outputs. A modified Matlab script is available for collecting Matlab outputs.

Note: "Run in Colab" button will be functional upon merging this PR.

@faridyagubbayli faridyagubbayli marked this pull request as draft October 8, 2023 16:15
@faridyagubbayli faridyagubbayli marked this pull request as ready for review November 5, 2023 17:19
Copy link
Owner

@waltsims waltsims left a comment

Choose a reason for hiding this comment

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

Hey Farid!

Amazing PR. The output plot of the notebook looks like it needs to be fixed.

@waltsims
Copy link
Owner

This might be the method we need to fix this example. I am leaving this comment for myself later.

@waltsims
Copy link
Owner

This might be the method we need to fix this example. I am leaving this comment for myself later.

This indeed worked.

@waltsims
Copy link
Owner

@faridyagubbayli please check again that get_color_map works as intended. perhaps we could make a test so we can test it in all python versions? I was having issues with python 3.8.

@waltsims waltsims self-requested a review January 15, 2024 05:16
Copy link
Owner

@waltsims waltsims Jan 15, 2024

Choose a reason for hiding this comment

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

Please make sure the example follows the passing proper usage and we're good!

@waltsims
Copy link
Owner

waltsims commented Jan 15, 2024

This might be the method we need to fix this example. I am leaving this comment for myself later.

This should be updated in the logic kWaveSimulation. There is an option there but it seems the logic doesn't work atm. Same in kwave? Let's update this logic on the next point release.

@faridyagubbayli
Copy link
Collaborator Author

@waltsims going to merge this soon, please let me know if I should not. I agree that we can fix the method you linked in the next point release.

@waltsims
Copy link
Owner

waltsims commented Jan 15, 2024

Im happy if you change the color map in the notebook and add 'update colormap' as an enhancement for the next point release. All examples will need it!

@waltsims
Copy link
Owner

I made the change. Just test the notebook please.

@waltsims waltsims added this to the v0.3.1 milestone Jan 16, 2024
@faridyagubbayli
Copy link
Collaborator Author

@waltsims thanks for making the change! I tested the notebook. Although colors are different, everything seems fine. Going to merge.

@faridyagubbayli faridyagubbayli merged commit edea8f5 into master Jan 16, 2024
22 checks passed
@faridyagubbayli faridyagubbayli deleted the example/na_controlling_the_pml branch January 16, 2024 10:02
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.

None yet

2 participants