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

Convert mode solver.py into a class #688

Merged
merged 1 commit into from
Mar 18, 2023

Conversation

weiliangjin2021
Copy link
Collaborator

All functions in solver.py have been converted into static method. The contents of the functions are untouched.

@weiliangjin2021
Copy link
Collaborator Author

Additional changes:

  • mode_solver_type = Literal["diagonal", "tensorial"] is passed to solver_eigs. They are unused in the local solver, but needed in the parallel mode solver for efficiently picking up the right eigenvalues.
  • A method eigs_to_effective_index is added to convert eigenvalues to the effective index. The motivation is the same as above.

@momchil-flex
Copy link
Collaborator

My only comment here is that there are currently at least two external libraries that have built upon our mode solver and use compute_modes directly. Those are gdsfactory, which @lucas-flexcompute should be updating to use the ModeSolver class directly at some point, but it hasn't yet been done I believe? and meow, Floris Laporte's EME sandbox that uses the tidy3d mode solver. Let's not break those for now (and in case other people that we don't know of are using compute_modes directly). Let's just leave the functions as they are in solver.py, and then underneath introduce the EigSolver as a thin wrapper, e.g.

class EigSolver(Tidy3dBaseModel):

    def compute_modes(*args, **kwargs):
        return compute_modes(*args, **kwargs)

What do you think?

By the way, could you remind me why all the methods are classmethods rather than regular methods?

@weiliangjin2021
Copy link
Collaborator Author

Let's just leave the functions as they are in solver.py, and then underneath introduce the EigSolver as a thin wrapper

That's a good idea.

By the way, could you remind me why all the methods are classmethods rather than regular methods?

The reason is that I want to override solver_eigs to not use scipy in the parallel mode solver. I don't find it easy to do with regular methods.

@momchil-flex
Copy link
Collaborator

I think I would rather just have it as compute_modes(*args, **kwargs) and with the docstring just saying that it's a wrapper around EigSolver.compute_modes, and the ModeSolver using EigSolver.compute_modes. This is with the idea of just removing compute_modes at some point in the future, and not having to modify things in two places meanwhile.

After that, feel free to merge, or keep open if you think further changes may be coming. Thanks!

@weiliangjin2021
Copy link
Collaborator Author

I think I would rather just have it as compute_modes(*args, **kwargs) and with the docstring just saying that it's a wrapper around EigSolver.compute_modes, and the ModeSolver using EigSolver.compute_modes.

If we have it as compute_modes(*args, **kwargs), what's the general rule of passing arguments to compute_modes? E.g. here mode_spec is a non-keyword argument, but in python method, people can pass it as keyword argument. Is there a standard procedure of getting arguments from *args, and **kwargs?

@weiliangjin2021
Copy link
Collaborator Author

I think I would rather just have it as compute_modes(*args, **kwargs) and with the docstring just saying that it's a wrapper around EigSolver.compute_modes, and the ModeSolver using EigSolver.compute_modes.

If we have it as compute_modes(*args, **kwargs), what's the general rule of passing arguments to compute_modes? E.g. here mode_spec is a non-keyword argument, but in python method, people can pass it as keyword argument. Is there a standard procedure of getting arguments from *args, and **kwargs?

Ah, i see what you mean.

@momchil-flex
Copy link
Collaborator

Yep, that's it!

For tensorial mode, switch to apply a real target eigvalue

Fix the ordering of modes
@weiliangjin2021 weiliangjin2021 merged commit 671326e into pre/1.10.0 Mar 18, 2023
@weiliangjin2021 weiliangjin2021 deleted the weiliang/mode_solver branch March 18, 2023 01:18
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