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

Forcing a scene to be loaded prior to defining radio materials has a bit of a code smell #216

Closed
jrounds opened this issue Sep 13, 2023 · 1 comment

Comments

@jrounds
Copy link

jrounds commented Sep 13, 2023

This errors on a fresh python instance

from sionna.rt import RadioMaterial
from sionna.rt import load_scene
#load_scene() # Load empty scene

custom_material = RadioMaterial("my_material",
                                relative_permittivity=2.0,
                                conductivity=5.0)

Error is Scene()._frequency is undefined because the init doesn't give it a default.

This does not error:

from sionna.rt import RadioMaterial
from sionna.rt import load_scene
load_scene() # Load empty scene

custom_material = RadioMaterial("my_material",
                                relative_permittivity=2.0,
                                conductivity=5.0)

It is clear from the API documentation that this is known. The api doc has the empty scene prior to defining the material.

The arrow of dependency is basically circular because of that reach in _frequency with no default on an undefined scene. Scene depends on materials and materials depend on Scene._frequency. Scene._frequency may change after materials are defined, so the necessity of this circular dependency is unclear.

From a high level there seems like there is a design where RadioMaterials can be defined independent of Scene and Scene uses them.

My interest is I just wanted some library of materials in a module. It is clear now though that to do so I will have to factory them after an initial scene load or do the load_scene() just to import, but I dont want to do that. Choosing factory method myself.

@jhoydis
Copy link
Collaborator

jhoydis commented Nov 29, 2023

This was resolved in release v0.16.

@jhoydis jhoydis closed this as completed Nov 29, 2023
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

No branches or pull requests

2 participants