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

Introduced RF material library to microwave plugins #1756

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

QimingFlex
Copy link
Contributor

Hey @weiliangjin2021 , I mirrored the optical material library to create an RF microwave material library and included three materials as examples. Could you please provide some feedback so I can refine the format of the RF material library?

Example:
from tidy3d import rf_material_library FR4 = rf_material_library['FR4'] print(FR4.medium)

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Thanks Qiming, this looks great! Two main comments:

  • A material can contain several variants. E.g. there can be several difference types of FR4 depending on the data source. Will be good to still follow this format?
  • It would be better to define those Abstract classes in tidy3d rather than in plugin. In that case, both RF material library and photonics material library items can subclass from them. This helps reduce redundant codes.

@@ -256,6 +256,7 @@
# get material `mat` and variant `var` as `material_library[mat][var]`
from .material_library.material_library import material_library
from .material_library.parametric_materials import Graphene
from .plugins.microwave.rf_material_library import rf_material_library
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to expose it to tidy3d namespace. Instead, add it to plugins/microwave/__init__.py

@QimingFlex
Copy link
Contributor Author

* A material can contain several variants. E.g. there can be several difference types of FR4 depending on the data source. Will be good to still follow this format?

Exactly, I will follow the optical library to include these variants, including the "design" and "process" variants we've discussed for the Rogers laminates.

* It would be better to define those Abstract classes in tidy3d rather than in plugin. In that case, both RF material library and photonics material library items can subclass from them. This helps reduce redundant codes.

Sounds good, I will handle it properly.

@QimingFlex
Copy link
Contributor Author

QimingFlex commented Jun 12, 2024

I introduced variants to the materials. Now, the RF materials follow a similar structure to the optical ones.
Example:

from tidy3d import rf_material_library

Rogers3010 = rf_material_library['Rogers3010']['design']

print(Rogers3010)

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

There are lots of formatting going on. Which ruff version are you using?

tidy3d/material_library/material_library.py Outdated Show resolved Hide resolved
@weiliangjin2021 weiliangjin2021 changed the base branch from pre/2.7 to develop June 12, 2024 18:46
@weiliangjin2021
Copy link
Collaborator

(also temporarily change the merge branch to develop, until we have pre/2.8 branch)

@weiliangjin2021
Copy link
Collaborator

The formatting here makes no different to ruff. I guess @QimingFlex format it with black? How does it work now with this new formatting setting @tylerflex ?

@tylerflex
Copy link
Collaborator

tylerflex commented Jun 12, 2024

@yaugenst recently updated the notion guide here for that.
https://www.notion.so/flexcompute/Tidy3D-Developer-Guide-60d5df306c2f4f29bea8c3b401ba6382?pvs=4#1737de671e154f96ae55690d2c96147b

I think just ruff format . should work

@tylerflex
Copy link
Collaborator

Also, recommend installing the pre-commit hooks to get this to happen automatically. I only recently installed it and it saves me a lot of time now pre-commit install from tidy3d/

@weiliangjin2021
Copy link
Collaborator

@yaugenst recently updated the notion guide here for that. https://www.notion.so/flexcompute/Tidy3D-Developer-Guide-60d5df306c2f4f29bea8c3b401ba6382?pvs=4#1737de671e154f96ae55690d2c96147b

I think just ruff format . should work

When running ruff format . on develop branch or this branch, it doesn't modify any files, while black will format one of the branch.

@QimingFlex
Copy link
Contributor Author

Yeah, I used both ruff --fix . and ruff format .. My ruff is version 0.3.5.

@tylerflex
Copy link
Collaborator

hm, I'm sorry, I actually haven't 100% figured out the best option. For me the pre-commit has been working well, without GitHub CI complaining, so maybe try installing that?

@weiliangjin2021
Copy link
Collaborator

hm, I'm sorry, I actually haven't 100% figured out the best option. For me the pre-commit has been working well, without GitHub CI complaining, so maybe try installing that?

My understanding is that ruff is not as strict as black, so it cannot discern the formatting difference between this and develop branch. In that case, pre-commit shouldn't help either, and GitHub CI will not complain about the formatting occurs in this branch, as both of them are based on ruff.

I guess just revert all formatting in this PR?

@yaugenst-flex
Copy link
Contributor

Hey all! The correct notion page is this one: https://www.notion.so/flexcompute/Tidy3D-Developer-Guide-23ceee49660e42fca06484bfcaa96b5c?pvs=4

I think everyone has access to that.

Regarding ruff: Everything on develop was formatted using ruff 0.4.8 - this is also pinned as a dependency, and is what the pre-commit hook uses. If you use anything else, i.e. older ruff versions or black, the formatting will be different and it will not pass in CI.

In general it's a good idea to keep a dev environment for each branch, and keep it up to date with the poetry lock file on that branch. I think @daquinteroflex set up the poetry packaging and wrote a guide on how to set up the env here.

Currently it's probably a bit confusing still how to set everything up, we should write a simple guide for this and communicate it to everyone...

@QimingFlex QimingFlex force-pushed the qiming/RFmaterial branch 2 times, most recently from f1d9cfe to 31088ab Compare June 13, 2024 21:43
@daquinteroflex
Copy link
Collaborator

daquinteroflex commented Jun 14, 2024

Hello! Yeah, agree with Yannick on what he mentions.

We should make a simpler version that is more straightforward and the one you've written on notion looks great! So yes, the idea of using the poetry environments guarantees we'll all have the same dependencies across our computers between PRs. Because it changed in the latest ruff update, that's why this formatting in this PR is failing as it was an older version. The pre-commit should streamline all of this for you without having to think about it after poetry run pre-commit install.

Happy to help you @QimingFlex if you need a hand too

@weiliangjin2021
Copy link
Collaborator

All look good! One remaining item is to add it to the doc. The photonics material library doc page is generated here.

@weiliangjin2021 weiliangjin2021 added the 2.8 will go into version 2.8.* label Jun 19, 2024
@weiliangjin2021 weiliangjin2021 changed the base branch from develop to pre/2.8 June 19, 2024 16:11
@QimingFlex
Copy link
Contributor Author

All look good! One remaining item is to add it to the doc. The photonics material library doc page is generated here.

Sure, I'm working on this!

@QimingFlex QimingFlex changed the base branch from pre/2.8 to develop June 20, 2024 20:18
@QimingFlex QimingFlex force-pushed the qiming/RFmaterial branch 2 times, most recently from 0665f16 to d77bd28 Compare June 20, 2024 20:27
@QimingFlex
Copy link
Contributor Author

@weiliangjin2021 generated the documentation and placed it under the folder tidy3d/api. Am I doing this correctly? How is the RST file transformed into a website?

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 generated the documentation and placed it under the folder tidy3d/api. Am I doing this correctly? How is the RST file transformed into a website?

Also need to include this RST in index.rst etc. E.g. photonics material library appears also here and here. How to build it into a website locally can be found here.

@QimingFlex QimingFlex force-pushed the qiming/RFmaterial branch 2 times, most recently from f58b3c9 to 8a0e380 Compare June 21, 2024 19:14
@QimingFlex QimingFlex changed the title [Draft] Introduced RF material library to microwave plugins Introduced RF material library to microwave plugins Jun 21, 2024
@QimingFlex QimingFlex marked this pull request as ready for review June 21, 2024 19:15
@QimingFlex QimingFlex force-pushed the qiming/RFmaterial branch 2 times, most recently from 9a9ff5d to 4189528 Compare June 21, 2024 19:46
@weiliangjin2021 weiliangjin2021 changed the base branch from develop to pre/2.8 June 24, 2024 23:16
@weiliangjin2021 weiliangjin2021 changed the base branch from pre/2.8 to develop June 24, 2024 23:16
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

All look good to me. Just rebase to pre/2.8.

@weiliangjin2021 weiliangjin2021 changed the base branch from develop to pre/2.8 June 25, 2024 22:07
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Thanks @QimingFlex ! Just a couple comments from me. Overall looks good!

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added
- Users can now export `SimulationData` to MATLAB `.mat` files with the `to_mat_file` method.
- Introduce RF material library including several commonly used RF PCB materials.
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be nice to mention where users can import this from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -227,8 +228,177 @@ def num2str(num):
f.write("\n")


def generate_rf_material_library_doc():
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this mostly copied from the genereate_material_library_doc? can we write in a way that we dont need to re-implement this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, mostly from generate_material_library_doc. I'll keep this in mind and reduce the redundancy later.

@QimingFlex QimingFlex merged commit 8d6c14c into pre/2.8 Jun 27, 2024
16 checks passed
@QimingFlex QimingFlex deleted the qiming/RFmaterial branch June 27, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 will go into version 2.8.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants