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

replace all "cmin" and "cmax" with "zmin" and "zmax" respectibly #546

Merged
merged 3 commits into from
Mar 6, 2025

Conversation

Stockless
Copy link
Collaborator

To improve consistency as requested in Issue #361 , every keyword "cmin/cmax" was replaced with "zmin/zmax" , as it was described with that notation in the keywords description for the plot_image function.

@cncastillo
Copy link
Member

cncastillo commented Mar 5, 2025

You probably need to redo this, the main problem is that

plot_image(...; zmin, zmax)

but

plot_phantom_map(...; cmin, cmax).

It doesn't really matter what they are called inside the functions, just the interface.

@Stockless
Copy link
Collaborator Author

It doesn't really matter what they are called inside the functions, just the interface.

in commit fa74970 I kept the change from cmin/cmax to zmin/zmax just when the plot_phantom_map function calls the kwargs and redefines zmin/zmax

@cncastillo
Copy link
Member

@Stockless Did you check if all the uses of plot_phantom_map use the new syntax?

@Stockless
Copy link
Collaborator Author

None of the uses of plot_phantom_map gives cmin/cmax as an argument, there won't be any issue with the new syntax. There's also no CI test error related to this change.

@cncastillo
Copy link
Member

Ok, add this a minor change for KomaMRIPlots to register it

@cncastillo cncastillo merged commit 15af96d into master Mar 6, 2025
6 of 11 checks passed
@cncastillo cncastillo deleted the script_corrections branch March 6, 2025 17:47
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