qpv-research-group / solcore5

A multi-scale, python-based library for the modelling of solar cells and semiconductor materials
https://www.solcore.solar/
Other
133 stars 77 forks source link

Remove seemingly unnecessary parsing of material name string #219

Closed phoebe-p closed 2 years ago

phoebe-p commented 2 years ago

Currently, when trying to get parameters using the material_system module, the get_parameters function 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 specifying material('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.

codecov[bot] commented 2 years ago

Codecov Report

Merging #219 (df5cc8a) into develop (c0c05aa) will decrease coverage by 0.10%. The diff coverage is 90.90%.

@@             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     
Impacted Files Coverage Δ
solcore/material_system/material_system.py 69.69% <ø> (-1.69%) :arrow_down:
solcore/absorption_calculator/adachi_alpha.py 76.28% <89.47%> (-1.38%) :arrow_down:
solcore/parameter_system/parameter_system.py 74.72% <100.00%> (-0.52%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

phoebe-p commented 2 years ago

Ok, have made the suggested changes and bumped version to 5.8.0.

phoebe-p commented 2 years ago

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.