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

Minor fixes: n/k database, updating examples, bugs in Cauchy model and material parameters #218

Merged
merged 5 commits into from Jun 1, 2022

Conversation

phoebe-p
Copy link
Member

@phoebe-p phoebe-p commented May 31, 2022

Few minor fixes:

  • Update which version of the refractiveindex.info database gets downloaded by default (was a version from 2018, now the most recent version). This changes the "pageid" which has to be used in some of the examples. In the example that is explicity about this I just updated the numbers and made a note that depending on the database version these may be different; in the 4J cell optimization example it is now fixed so that it will find the right entry regardless of database version. These examples have also been updated in the documentation
  • The Cauchy model to calculate the refractive index had a bug (final term was C*wavelength^4 instead of C/wavelength^4. This is fixed.
  • I am not sure if this was intentional, but for Si and Ge there was an entry in their parameter files for relative_dielectric_constant rather than relative_permittivity. relative_dielectric_constant is never actually used by Solcore (unlike relative_permittivity) and as far as I understand it is the same thing anyway. This means that if you used Si and Ge you had to set this parameter manually, which seems unnecessary given that it is well-known, so I have changed these entries to relative_permittivity.
  • Similar to the issue fixed in this commit, now also fixed in the PDD solver; if you did not explicity set an incident spectrum, but did specify the wavelengths, then your user-defined wavelength would get overwritten when calculating the spectrum in short_circuit_pdd

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #218 (80ff2b1) into develop (b8b8b44) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #218   +/-   ##
========================================
  Coverage    45.85%   45.86%           
========================================
  Files           84       84           
  Lines         9100     9101    +1     
========================================
+ Hits          4173     4174    +1     
  Misses        4927     4927           
Impacted Files Coverage Δ
solcore/absorption_calculator/nk_db.py 17.74% <ø> (ø)
solcore/solar_cell_solver.py 66.46% <ø> (ø)
...bsorption_calculator/dielectric_constant_models.py 75.32% <100.00%> (+0.16%) ⬆️
...erial_data/refractiveindex_info_DB/dboperations.py 14.13% <100.00%> (ø)
...poisson_drift_diffusion/DriftDiffusionUtilities.py 86.47% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8b8b44...80ff2b1. Read the comment docs.

@phoebe-p phoebe-p requested a review from dalonsoa May 31, 2022 07:48
Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

All good! Well spotted errors and updates.

x_eV = 1240 / x

N = self.An + self.Bn / (x / 1000.) ** 2 + self.Cn * (x / 1000.) ** 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering the severity of the error, it suggests this model has not been used that often or someone should have spotted the mistake long ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I tried to actually use it and it was immediately obvious that something was wrong, but that was the first time I (and maybe anyone) had actually tried to use it...

solcore/material_data/Levinshtein/GroupIV.txt Show resolved Hide resolved
@@ -15,7 +15,7 @@
Entry = namedtuple('Entry',['id','shelf','book','page'])

# Check latest available database in https://refractiveindex.info/download.php
_riiurl = "https://refractiveindex.info/download/database/rii-database-2018-07-01.zip"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish there were a more programatic way of checking the database data and fetching the last version. Hardcoding it looks a bit hacky.

Copy link
Member Author

Choose a reason for hiding this comment

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

There probably is, I just haven't looked into it! I will leave it like this now but will have a look to see if there is a more elegant way I can do it.

@phoebe-p phoebe-p merged commit c0c05aa into develop Jun 1, 2022
@phoebe-p phoebe-p deleted the local_changes branch February 13, 2023 05:20
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.

None yet

2 participants