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 separated component field plotting #132

Merged
merged 24 commits into from
Mar 7, 2024

Conversation

lucasgrjn
Copy link
Contributor

Solve the issue #90 from @lukasc-ubc

Implement plot_component to allow the user to set the field, the direction and the part (real, imaginary or absolute value) to be plotted.

@HelgeGehring
Copy link
Owner

Looks great!
How could we keep it backwards compatible? (At least for the next few versions with a deprecation warning)

@lucasgrjn
Copy link
Contributor Author

Looks great! How could we keep it backwards compatible? (At least for the next few versions with a deprecation warning)

Good idea! I will take a look into that

@lucasgrjn
Copy link
Contributor Author

I see to possible way:

  1. the easiest will be to replace the actual .show() by another function like .plot()
  2. just to get some keywords and throw the deprecation warning depending on which one

I prefer the first one since it will enforce people to "learn" to use the new function. What do you think?

@lucasgrjn lucasgrjn closed this Mar 4, 2024
@lucasgrjn
Copy link
Contributor Author

Now, with the last commits, the function should be backward compatible!

@lucasgrjn lucasgrjn reopened this Mar 4, 2024
Copy link
Owner

@HelgeGehring HelgeGehring left a comment

Choose a reason for hiding this comment

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

Looks amazing! Just some minor tweaks and then we can merge!

femwell/maxwell/waveguide.py Outdated Show resolved Hide resolved
femwell/maxwell/waveguide.py Outdated Show resolved Hide resolved
femwell/maxwell/waveguide.py Outdated Show resolved Hide resolved
femwell/maxwell/waveguide.py Outdated Show resolved Hide resolved
Copy link
Owner

@HelgeGehring HelgeGehring left a comment

Choose a reason for hiding this comment

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

Looks amazing, I'm soo close to approving!

femwell/maxwell/waveguide.py Show resolved Hide resolved
femwell/maxwell/waveguide.py Outdated Show resolved Hide resolved
boundaries: bool = True,
colorbar: bool = False,
direction: Literal["x", "y"] = "x",
title: str = "E",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
title: str = "E",
title: Optional[str] = None,

That way we could use field (if it's a Literal) as the title in case the user doesn't define anything?

Copy link
Owner

Choose a reason for hiding this comment

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

just as a small extra: when you pass it on to the new function, could you add there for the title

title = field if title is None else title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this one. If the user does not want to add a title, should we really enforce one?

femwell/maxwell/waveguide.py Show resolved Hide resolved
Copy link
Owner

@HelgeGehring HelgeGehring left a comment

Choose a reason for hiding this comment

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

I think like this we get titles for all the fields in case we don't set anything ,right?

femwell/maxwell/waveguide.py Show resolved Hide resolved
femwell/maxwell/waveguide.py Outdated Show resolved Hide resolved
@HelgeGehring
Copy link
Owner

What do you think? I think once we add that we're done?

@HelgeGehring HelgeGehring merged commit eaf36ae into HelgeGehring:main Mar 7, 2024
5 checks 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