Remove seemingly unnecessary parsing of material name string #219
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #219 +/- ##
===========================================
- Coverage 45.86% 45.75% -0.11%
===========================================
Files 84 84
Lines 9101 9090 -11
===========================================
- Hits 4174 4159 -15
- Misses 4927 4931 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Main change is related to update the dosctrings.
Having said that, the fact that the approach you are removing is not used in the tests does not mean that the end users are not using it. I agree it is unnecessary and should be removed, but it is also a breaking change that might affect other people.
I suggest the following:
- Add a warning that pops up when using
get_parameterexplaining the change and how it needs to be used now. - Move the version number one step, to 5.8.0
| @@ -53,6 +53,7 @@ def __init__(self, sources: Optional[Callable] = None): | |||
| "([A-Z][a-z]*)") | |||
|
|
|||
| def get_parameter(self, material, parameter, verbose=False, **others): | |||
|
|
|||
There was a problem hiding this comment.
Why is there a blank line here?
Anyway, since this functionhas changed its behavior - now providing "In0.2GaAsP0.1" will not be enough to retrieve the material, the docstring will need to be updated explaining what it expect for inputs.
There was a problem hiding this comment.
Blank line was accidental... I have updated the docstring and added a DeprecationWarning.
|
Ok, have made the suggested changes and bumped version to 5.8.0. |
|
Good catch, thanks - I have just removed checking for the string. Hopefully this does not break functionality anyone is currently using, but in order to look up the parameters in that function the material should be in the database anyway, so even if anyone is passing the name as a string it should be a simple fix. |
Currently, when trying to get parameters using the material_system module, the
get_parametersfunction parses the material string, strips out the numbers (to pass them as alloy fractions), and then tries to look up the parameter. This works as expected for Solcore built-in materials, and custom materials which do not have numbers in the name. However, if you define a custom material with numbers in the name (for instance 'Al0.8Ga0.2AsP'), providing not just the optical constant but also further material parameters like bandgaps and effective masses, an error will happen since the numbers will get stripped out and get_parameter attempts to look up parameters under the heading 'AlGaAsP', which is not in the database (and even if it was in the database, it would not be the material entry we want to get the parameter from!).This behaviour seems to exist so that you could define a material as something like
material('In0.1Ga0.9As')and let Solcore figure out the alloy fractions, rather than specifyingmaterial('InGaAs')(In=0.1)but actually this sort of usage doesn't happen in any current tests or examples (maybe there is a historical reason for it?). Removing this step (stripping out the numbers from the material name) altogether currently only breaks the create_adachi_alpha function, which should be pretty easy to fix. I just want to make sure there isn't some deeper reason for this behaviour which I am not aware of.